There can be error conditions within Intel BT firmware that can cause it
to get stuck, with the only way out being toggle the reset pin to the
device. (I do not have the details about the issues that lead to such
conditions, Intel folks copied here can elaborate if needed). Thus, this
is an effor to be able to toggle the reset line from the driver if it
detects such a situation. It makes few enhancements to the common
framework which I think may be useful for other unrelated problems.
Dmitry Torokhov (2):
usb: split code locating ACPI companion into port and device
usb: assign ACPI companions for embedded USB devices
(This basically allows ACPI nodes to be attached to the USB devices,
thus useful for any onboard / embedded USB devices that wants to get
some info from the ACPI).
Rajat Jain (3):
Bluetooth: Reset Bluetooth chip after multiple command timeouts
Bluetooth: btusb: Collect the common Intel assignments together
Bluetooth: btusb: Use the hw_reset method to allow resetting the BT
chip
drivers/bluetooth/btusb.c | 63 +++++++++---
drivers/usb/core/usb-acpi.c | 163 +++++++++++++++++++------------
include/net/bluetooth/hci.h | 8 ++
include/net/bluetooth/hci_core.h | 2 +
net/bluetooth/hci_core.c | 15 ++-
5 files changed, 171 insertions(+), 80 deletions(-)
--
2.19.1.1215.g8438c0b245-goog
From: Dmitry Torokhov <[email protected]>
In preparation for handling embedded USB devices let's split
usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and
usb_acpi_find_companion_for_port().
Signed-off-by: Dmitry Torokhov <[email protected]>
Signed-off-by: Rajat Jain <[email protected]>
---
drivers/usb/core/usb-acpi.c | 133 +++++++++++++++++++-----------------
1 file changed, 72 insertions(+), 61 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index e221861b3187..8ff73c83e8e8 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -139,12 +139,79 @@ static struct acpi_device *usb_acpi_find_port(struct acpi_device *parent,
return acpi_find_child_device(parent, raw, false);
}
-static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+static struct acpi_device *
+usb_acpi_get_companion_for_port(struct usb_port *port_dev)
{
struct usb_device *udev;
struct acpi_device *adev;
acpi_handle *parent_handle;
+ int port1;
+
+ /* Get the struct usb_device point of port's hub */
+ udev = to_usb_device(port_dev->dev.parent->parent);
+
+ /*
+ * The root hub ports' parent is the root hub. The non-root-hub
+ * ports' parent is the parent hub port which the hub is
+ * connected to.
+ */
+ if (!udev->parent) {
+ adev = ACPI_COMPANION(&udev->dev);
+ port1 = usb_hcd_find_raw_port_number(bus_to_hcd(udev->bus),
+ port_dev->portnum);
+ } else {
+ parent_handle = usb_get_hub_port_acpi_handle(udev->parent,
+ udev->portnum);
+ if (!parent_handle)
+ return NULL;
+
+ acpi_bus_get_device(parent_handle, &adev);
+ port1 = port_dev->portnum;
+ }
+
+ return usb_acpi_find_port(adev, port1);
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_port(struct usb_port *port_dev)
+{
+ struct acpi_device *adev;
+ struct acpi_pld_info *pld;
+ acpi_handle *handle;
+ acpi_status status;
+
+ adev = usb_acpi_get_companion_for_port(port_dev);
+ if (!adev)
+ return NULL;
+
+ handle = adev->handle;
+ status = acpi_get_physical_device_location(handle, &pld);
+ if (!ACPI_FAILURE(status) && pld) {
+ port_dev->location = USB_ACPI_LOCATION_VALID
+ | pld->group_token << 8 | pld->group_position;
+ port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
+ ACPI_FREE(pld);
+ }
+ return adev;
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_device(struct usb_device *udev)
+{
+ struct acpi_device *adev;
+
+ if (!udev->parent)
+ return NULL;
+
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+}
+
+
+static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+{
/*
* In the ACPI DSDT table, only usb root hub and usb ports are
* acpi device nodes. The hierarchy like following.
@@ -158,66 +225,10 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
* So all binding process is divided into two parts. binding
* root hub and usb ports.
*/
- if (is_usb_device(dev)) {
- udev = to_usb_device(dev);
- if (udev->parent)
- return NULL;
-
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(dev->parent);
- return acpi_find_child_device(adev, 0, false);
- } else if (is_usb_port(dev)) {
- struct usb_port *port_dev = to_usb_port(dev);
- int port1 = port_dev->portnum;
- struct acpi_pld_info *pld;
- acpi_handle *handle;
- acpi_status status;
-
- /* Get the struct usb_device point of port's hub */
- udev = to_usb_device(dev->parent->parent);
-
- /*
- * The root hub ports' parent is the root hub. The non-root-hub
- * ports' parent is the parent hub port which the hub is
- * connected to.
- */
- if (!udev->parent) {
- struct usb_hcd *hcd = bus_to_hcd(udev->bus);
- int raw;
-
- raw = usb_hcd_find_raw_port_number(hcd, port1);
-
- adev = usb_acpi_find_port(ACPI_COMPANION(&udev->dev),
- raw);
-
- if (!adev)
- return NULL;
- } else {
- parent_handle =
- usb_get_hub_port_acpi_handle(udev->parent,
- udev->portnum);
- if (!parent_handle)
- return NULL;
-
- acpi_bus_get_device(parent_handle, &adev);
-
- adev = usb_acpi_find_port(adev, port1);
-
- if (!adev)
- return NULL;
- }
- handle = adev->handle;
- status = acpi_get_physical_device_location(handle, &pld);
- if (ACPI_FAILURE(status) || !pld)
- return adev;
-
- port_dev->location = USB_ACPI_LOCATION_VALID
- | pld->group_token << 8 | pld->group_position;
- port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
- ACPI_FREE(pld);
-
- return adev;
- }
+ if (is_usb_device(dev))
+ return usb_acpi_find_companion_for_device(to_usb_device(dev));
+ else if (is_usb_port(dev))
+ return usb_acpi_find_companion_for_port(to_usb_port(dev));
return NULL;
}
--
2.19.1.1215.g8438c0b245-goog
From: Dmitry Torokhov <[email protected]>
USB devices permanently connected to USB ports may be described in ACPI
tables and share ACPI devices with ports they are connected to. See [1]
for details.
This will allow us to describe sideband resources for devices, such as,
for example, hard reset line for BT USB controllers.
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects#acpi-namespace-hierarchy-and-adr-for-embedded-usb-devices
Signed-off-by: Dmitry Torokhov <[email protected]>
Signed-off-by: Rajat Jain <[email protected]> (changed how we get the usb_port)
---
drivers/usb/core/usb-acpi.c | 44 +++++++++++++++++++++++++++++--------
1 file changed, 35 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 8ff73c83e8e8..9043d7242d67 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -200,30 +200,56 @@ static struct acpi_device *
usb_acpi_find_companion_for_device(struct usb_device *udev)
{
struct acpi_device *adev;
+ struct usb_port *port_dev;
+ struct usb_hub *hub;
+
+ if (!udev->parent) {
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+ }
- if (!udev->parent)
+ hub = usb_hub_to_struct_hub(udev->parent);
+ if (!hub)
return NULL;
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(udev->dev.parent);
- return acpi_find_child_device(adev, 0, false);
+ /*
+ * This is an embedded USB device connected to a port and such
+ * devices share port's ACPI companion.
+ */
+ port_dev = hub->ports[udev->portnum - 1];
+ return usb_acpi_get_companion_for_port(port_dev);
}
-
static struct acpi_device *usb_acpi_find_companion(struct device *dev)
{
/*
- * In the ACPI DSDT table, only usb root hub and usb ports are
- * acpi device nodes. The hierarchy like following.
+ * The USB hierarchy like following:
+ *
* Device (EHC1)
* Device (HUBN)
* Device (PR01)
* Device (PR11)
* Device (PR12)
+ * Device (FN12)
+ * Device (FN13)
* Device (PR13)
* ...
- * So all binding process is divided into two parts. binding
- * root hub and usb ports.
+ * where HUBN is root hub, and PRNN are USB ports and devices
+ * connected to them, and FNNN are individualk functions for
+ * connected composite USB devices. PRNN and FNNN may contain
+ * _CRS and other methods describing sideband resources for
+ * the connected device.
+ *
+ * On the kernel side both root hub and embedded USB devices are
+ * represented as instances of usb_device structure, and ports
+ * are represented as usb_port structures, so the whole process
+ * is split into 2 parts: finding companions for devices and
+ * finding companions for ports.
+ *
+ * Note that we do not handle individual functions of composite
+ * devices yet, for that we would need to assign companions to
+ * devices corresponding to USB interfaces.
*/
if (is_usb_device(dev))
return usb_acpi_find_companion_for_device(to_usb_device(dev));
--
2.19.1.1215.g8438c0b245-goog
Add a quirk and a hook to allow the HCI core to reset the BT chip
if needed (after a number of timed out commands). Use that new hook to
initiate BT chip reset if the controller fails to respond to certain
number of commands (currently 5) including the HCI reset commands.
This is done based on a newly introduced quirk. This is done based
on some initial work by Intel.
Signed-off-by: Rajat Jain <[email protected]>
---
include/net/bluetooth/hci.h | 8 ++++++++
include/net/bluetooth/hci_core.h | 2 ++
net/bluetooth/hci_core.c | 15 +++++++++++++--
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c36dc1e20556..af02fa5ffe54 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -192,6 +192,14 @@ enum {
*
*/
HCI_QUIRK_NON_PERSISTENT_SETUP,
+
+ /* When this quirk is set, hw_reset() would be run to reset the
+ * hardware, after a certain number of commands (currently 5)
+ * time out because the device fails to respond.
+ *
+ * This quirk should be set before hci_register_dev is called.
+ */
+ HCI_QUIRK_HW_RESET_ON_TIMEOUT,
};
/* HCI device flags */
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e5ea633ea368..b86218304b80 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -313,6 +313,7 @@ struct hci_dev {
unsigned int acl_cnt;
unsigned int sco_cnt;
unsigned int le_cnt;
+ unsigned int timeout_cnt;
unsigned int acl_mtu;
unsigned int sco_mtu;
@@ -437,6 +438,7 @@ struct hci_dev {
int (*post_init)(struct hci_dev *hdev);
int (*set_diag)(struct hci_dev *hdev, bool enable);
int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
+ void (*hw_reset)(struct hci_dev *hdev);
};
#define HCI_PHY_HANDLE(handle) (handle & 0xff)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7352fe85674b..ab3a6a8b7ba6 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2569,13 +2569,24 @@ static void hci_cmd_timeout(struct work_struct *work)
struct hci_dev *hdev = container_of(work, struct hci_dev,
cmd_timer.work);
+ hdev->timeout_cnt++;
if (hdev->sent_cmd) {
struct hci_command_hdr *sent = (void *) hdev->sent_cmd->data;
u16 opcode = __le16_to_cpu(sent->opcode);
- bt_dev_err(hdev, "command 0x%4.4x tx timeout", opcode);
+ bt_dev_err(hdev, "command 0x%4.4x tx timeout (cnt = %u)",
+ opcode, hdev->timeout_cnt);
} else {
- bt_dev_err(hdev, "command tx timeout");
+ bt_dev_err(hdev, "command tx timeout (cnt = %u)",
+ hdev->timeout_cnt);
+ }
+
+ if (test_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks) &&
+ hdev->timeout_cnt >= 5) {
+ hdev->timeout_cnt = 0;
+ if (hdev->hw_reset)
+ hdev->hw_reset(hdev);
+ return;
}
atomic_set(&hdev->cmd_cnt, 1);
--
2.19.1.1215.g8438c0b245-goog
The BTUSB_INTEL and BTUSB_INTEL_NEW have common functions & quirks are
assigned to hdev structure. Lets collect them together instead of
repeating them in different code branches.
Signed-off-by: Rajat Jain <[email protected]>
---
drivers/bluetooth/btusb.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 7439a7eb50ac..e8e148480c91 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3077,28 +3077,25 @@ static int btusb_probe(struct usb_interface *intf,
data->diag = usb_ifnum_to_if(data->udev, ifnum_base + 2);
}
#endif
+ if (id->driver_info & BTUSB_INTEL ||
+ id->driver_info & BTUSB_INTEL_NEW) {
- if (id->driver_info & BTUSB_INTEL) {
hdev->manufacturer = 2;
- hdev->setup = btusb_setup_intel;
- hdev->shutdown = btusb_shutdown_intel;
- hdev->set_diag = btintel_set_diag_mfg;
hdev->set_bdaddr = btintel_set_bdaddr;
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 (id->driver_info & BTUSB_INTEL_NEW) {
- hdev->manufacturer = 2;
- hdev->send = btusb_send_frame_intel;
- hdev->setup = btusb_setup_intel_new;
- hdev->hw_error = btintel_hw_error;
- hdev->set_diag = btintel_set_diag;
- hdev->set_bdaddr = btintel_set_bdaddr;
- 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 (id->driver_info & BTUSB_INTEL) {
+ hdev->setup = btusb_setup_intel;
+ hdev->shutdown = btusb_shutdown_intel;
+ hdev->set_diag = btintel_set_diag_mfg;
+ } else {
+ hdev->send = btusb_send_frame_intel;
+ hdev->setup = btusb_setup_intel_new;
+ hdev->hw_error = btintel_hw_error;
+ hdev->set_diag = btintel_set_diag;
+ }
}
if (id->driver_info & BTUSB_MARVELL)
--
2.19.1.1215.g8438c0b245-goog
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 <[email protected]>
---
drivers/bluetooth/btusb.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index e8e148480c91..8aad02d9e211 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,28 @@ 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.
+ */
+ if (reset_gpio) {
+ 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;
@@ -3030,6 +3055,11 @@ static int btusb_probe(struct usb_interface *intf,
SET_HCIDEV_DEV(hdev, &intf->dev);
+ data->reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
+ GPIOD_OUT_LOW);
+ if (data->reset_gpio)
+ hdev->hw_reset = btusb_hw_reset;
+
hdev->open = btusb_open;
hdev->close = btusb_close;
hdev->flush = btusb_flush;
@@ -3085,6 +3115,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 +3256,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 +3301,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
From: Dmitry Torokhov <[email protected]>
In preparation for handling embedded USB devices let's split
usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and
usb_acpi_find_companion_for_port().
Signed-off-by: Dmitry Torokhov <[email protected]>
Signed-off-by: Rajat Jain <[email protected]>
---
v2: same as v1
drivers/usb/core/usb-acpi.c | 133 +++++++++++++++++++-----------------
1 file changed, 72 insertions(+), 61 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index e221861b3187..8ff73c83e8e8 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -139,12 +139,79 @@ static struct acpi_device *usb_acpi_find_port(struct acpi_device *parent,
return acpi_find_child_device(parent, raw, false);
}
-static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+static struct acpi_device *
+usb_acpi_get_companion_for_port(struct usb_port *port_dev)
{
struct usb_device *udev;
struct acpi_device *adev;
acpi_handle *parent_handle;
+ int port1;
+
+ /* Get the struct usb_device point of port's hub */
+ udev = to_usb_device(port_dev->dev.parent->parent);
+
+ /*
+ * The root hub ports' parent is the root hub. The non-root-hub
+ * ports' parent is the parent hub port which the hub is
+ * connected to.
+ */
+ if (!udev->parent) {
+ adev = ACPI_COMPANION(&udev->dev);
+ port1 = usb_hcd_find_raw_port_number(bus_to_hcd(udev->bus),
+ port_dev->portnum);
+ } else {
+ parent_handle = usb_get_hub_port_acpi_handle(udev->parent,
+ udev->portnum);
+ if (!parent_handle)
+ return NULL;
+
+ acpi_bus_get_device(parent_handle, &adev);
+ port1 = port_dev->portnum;
+ }
+
+ return usb_acpi_find_port(adev, port1);
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_port(struct usb_port *port_dev)
+{
+ struct acpi_device *adev;
+ struct acpi_pld_info *pld;
+ acpi_handle *handle;
+ acpi_status status;
+
+ adev = usb_acpi_get_companion_for_port(port_dev);
+ if (!adev)
+ return NULL;
+
+ handle = adev->handle;
+ status = acpi_get_physical_device_location(handle, &pld);
+ if (!ACPI_FAILURE(status) && pld) {
+ port_dev->location = USB_ACPI_LOCATION_VALID
+ | pld->group_token << 8 | pld->group_position;
+ port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
+ ACPI_FREE(pld);
+ }
+ return adev;
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_device(struct usb_device *udev)
+{
+ struct acpi_device *adev;
+
+ if (!udev->parent)
+ return NULL;
+
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+}
+
+
+static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+{
/*
* In the ACPI DSDT table, only usb root hub and usb ports are
* acpi device nodes. The hierarchy like following.
@@ -158,66 +225,10 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
* So all binding process is divided into two parts. binding
* root hub and usb ports.
*/
- if (is_usb_device(dev)) {
- udev = to_usb_device(dev);
- if (udev->parent)
- return NULL;
-
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(dev->parent);
- return acpi_find_child_device(adev, 0, false);
- } else if (is_usb_port(dev)) {
- struct usb_port *port_dev = to_usb_port(dev);
- int port1 = port_dev->portnum;
- struct acpi_pld_info *pld;
- acpi_handle *handle;
- acpi_status status;
-
- /* Get the struct usb_device point of port's hub */
- udev = to_usb_device(dev->parent->parent);
-
- /*
- * The root hub ports' parent is the root hub. The non-root-hub
- * ports' parent is the parent hub port which the hub is
- * connected to.
- */
- if (!udev->parent) {
- struct usb_hcd *hcd = bus_to_hcd(udev->bus);
- int raw;
-
- raw = usb_hcd_find_raw_port_number(hcd, port1);
-
- adev = usb_acpi_find_port(ACPI_COMPANION(&udev->dev),
- raw);
-
- if (!adev)
- return NULL;
- } else {
- parent_handle =
- usb_get_hub_port_acpi_handle(udev->parent,
- udev->portnum);
- if (!parent_handle)
- return NULL;
-
- acpi_bus_get_device(parent_handle, &adev);
-
- adev = usb_acpi_find_port(adev, port1);
-
- if (!adev)
- return NULL;
- }
- handle = adev->handle;
- status = acpi_get_physical_device_location(handle, &pld);
- if (ACPI_FAILURE(status) || !pld)
- return adev;
-
- port_dev->location = USB_ACPI_LOCATION_VALID
- | pld->group_token << 8 | pld->group_position;
- port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
- ACPI_FREE(pld);
-
- return adev;
- }
+ if (is_usb_device(dev))
+ return usb_acpi_find_companion_for_device(to_usb_device(dev));
+ else if (is_usb_port(dev))
+ return usb_acpi_find_companion_for_port(to_usb_port(dev));
return NULL;
}
--
2.19.1.1215.g8438c0b245-goog
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 <[email protected]>
---
v2: Handle the EPROBE_DEFER case.
drivers/bluetooth/btusb.c | 42 +++++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index e8e148480c91..bf522cfe68c1 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,28 @@ 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.
+ */
+ if (reset_gpio) {
+ 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 +2942,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 +3056,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 (PTR_ERR(reset_gpio) == -EPROBE_DEFER) {
+ err = -EPROBE_DEFER;
+ goto out_free_dev;
+ } else if (!IS_ERR(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 +3121,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 +3262,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 +3307,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
From: Dmitry Torokhov <[email protected]>
USB devices permanently connected to USB ports may be described in ACPI
tables and share ACPI devices with ports they are connected to. See [1]
for details.
This will allow us to describe sideband resources for devices, such as,
for example, hard reset line for BT USB controllers.
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects#acpi-namespace-hierarchy-and-adr-for-embedded-usb-devices
Signed-off-by: Dmitry Torokhov <[email protected]>
Signed-off-by: Rajat Jain <[email protected]> (changed how we get the usb_port)
---
v2: same as v1
drivers/usb/core/usb-acpi.c | 44 +++++++++++++++++++++++++++++--------
1 file changed, 35 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 8ff73c83e8e8..9043d7242d67 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -200,30 +200,56 @@ static struct acpi_device *
usb_acpi_find_companion_for_device(struct usb_device *udev)
{
struct acpi_device *adev;
+ struct usb_port *port_dev;
+ struct usb_hub *hub;
+
+ if (!udev->parent) {
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+ }
- if (!udev->parent)
+ hub = usb_hub_to_struct_hub(udev->parent);
+ if (!hub)
return NULL;
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(udev->dev.parent);
- return acpi_find_child_device(adev, 0, false);
+ /*
+ * This is an embedded USB device connected to a port and such
+ * devices share port's ACPI companion.
+ */
+ port_dev = hub->ports[udev->portnum - 1];
+ return usb_acpi_get_companion_for_port(port_dev);
}
-
static struct acpi_device *usb_acpi_find_companion(struct device *dev)
{
/*
- * In the ACPI DSDT table, only usb root hub and usb ports are
- * acpi device nodes. The hierarchy like following.
+ * The USB hierarchy like following:
+ *
* Device (EHC1)
* Device (HUBN)
* Device (PR01)
* Device (PR11)
* Device (PR12)
+ * Device (FN12)
+ * Device (FN13)
* Device (PR13)
* ...
- * So all binding process is divided into two parts. binding
- * root hub and usb ports.
+ * where HUBN is root hub, and PRNN are USB ports and devices
+ * connected to them, and FNNN are individualk functions for
+ * connected composite USB devices. PRNN and FNNN may contain
+ * _CRS and other methods describing sideband resources for
+ * the connected device.
+ *
+ * On the kernel side both root hub and embedded USB devices are
+ * represented as instances of usb_device structure, and ports
+ * are represented as usb_port structures, so the whole process
+ * is split into 2 parts: finding companions for devices and
+ * finding companions for ports.
+ *
+ * Note that we do not handle individual functions of composite
+ * devices yet, for that we would need to assign companions to
+ * devices corresponding to USB interfaces.
*/
if (is_usb_device(dev))
return usb_acpi_find_companion_for_device(to_usb_device(dev));
--
2.19.1.1215.g8438c0b245-goog
The BTUSB_INTEL and BTUSB_INTEL_NEW have common functions & quirks are
assigned to hdev structure. Lets collect them together instead of
repeating them in different code branches.
Signed-off-by: Rajat Jain <[email protected]>
---
v2: same as v1
drivers/bluetooth/btusb.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 7439a7eb50ac..e8e148480c91 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3077,28 +3077,25 @@ static int btusb_probe(struct usb_interface *intf,
data->diag = usb_ifnum_to_if(data->udev, ifnum_base + 2);
}
#endif
+ if (id->driver_info & BTUSB_INTEL ||
+ id->driver_info & BTUSB_INTEL_NEW) {
- if (id->driver_info & BTUSB_INTEL) {
hdev->manufacturer = 2;
- hdev->setup = btusb_setup_intel;
- hdev->shutdown = btusb_shutdown_intel;
- hdev->set_diag = btintel_set_diag_mfg;
hdev->set_bdaddr = btintel_set_bdaddr;
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 (id->driver_info & BTUSB_INTEL_NEW) {
- hdev->manufacturer = 2;
- hdev->send = btusb_send_frame_intel;
- hdev->setup = btusb_setup_intel_new;
- hdev->hw_error = btintel_hw_error;
- hdev->set_diag = btintel_set_diag;
- hdev->set_bdaddr = btintel_set_bdaddr;
- 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 (id->driver_info & BTUSB_INTEL) {
+ hdev->setup = btusb_setup_intel;
+ hdev->shutdown = btusb_shutdown_intel;
+ hdev->set_diag = btintel_set_diag_mfg;
+ } else {
+ hdev->send = btusb_send_frame_intel;
+ hdev->setup = btusb_setup_intel_new;
+ hdev->hw_error = btintel_hw_error;
+ hdev->set_diag = btintel_set_diag;
+ }
}
if (id->driver_info & BTUSB_MARVELL)
--
2.19.1.1215.g8438c0b245-goog
Add a quirk and a hook to allow the HCI core to reset the BT chip
if needed (after a number of timed out commands). Use that new hook to
initiate BT chip reset if the controller fails to respond to certain
number of commands (currently 5) including the HCI reset commands.
This is done based on a newly introduced quirk. This is done based
on some initial work by Intel.
Signed-off-by: Rajat Jain <[email protected]>
---
v2: same as v1
include/net/bluetooth/hci.h | 8 ++++++++
include/net/bluetooth/hci_core.h | 2 ++
net/bluetooth/hci_core.c | 15 +++++++++++++--
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c36dc1e20556..af02fa5ffe54 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -192,6 +192,14 @@ enum {
*
*/
HCI_QUIRK_NON_PERSISTENT_SETUP,
+
+ /* When this quirk is set, hw_reset() would be run to reset the
+ * hardware, after a certain number of commands (currently 5)
+ * time out because the device fails to respond.
+ *
+ * This quirk should be set before hci_register_dev is called.
+ */
+ HCI_QUIRK_HW_RESET_ON_TIMEOUT,
};
/* HCI device flags */
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e5ea633ea368..b86218304b80 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -313,6 +313,7 @@ struct hci_dev {
unsigned int acl_cnt;
unsigned int sco_cnt;
unsigned int le_cnt;
+ unsigned int timeout_cnt;
unsigned int acl_mtu;
unsigned int sco_mtu;
@@ -437,6 +438,7 @@ struct hci_dev {
int (*post_init)(struct hci_dev *hdev);
int (*set_diag)(struct hci_dev *hdev, bool enable);
int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
+ void (*hw_reset)(struct hci_dev *hdev);
};
#define HCI_PHY_HANDLE(handle) (handle & 0xff)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7352fe85674b..ab3a6a8b7ba6 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2569,13 +2569,24 @@ static void hci_cmd_timeout(struct work_struct *work)
struct hci_dev *hdev = container_of(work, struct hci_dev,
cmd_timer.work);
+ hdev->timeout_cnt++;
if (hdev->sent_cmd) {
struct hci_command_hdr *sent = (void *) hdev->sent_cmd->data;
u16 opcode = __le16_to_cpu(sent->opcode);
- bt_dev_err(hdev, "command 0x%4.4x tx timeout", opcode);
+ bt_dev_err(hdev, "command 0x%4.4x tx timeout (cnt = %u)",
+ opcode, hdev->timeout_cnt);
} else {
- bt_dev_err(hdev, "command tx timeout");
+ bt_dev_err(hdev, "command tx timeout (cnt = %u)",
+ hdev->timeout_cnt);
+ }
+
+ if (test_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks) &&
+ hdev->timeout_cnt >= 5) {
+ hdev->timeout_cnt = 0;
+ if (hdev->hw_reset)
+ hdev->hw_reset(hdev);
+ return;
}
atomic_set(&hdev->cmd_cnt, 1);
--
2.19.1.1215.g8438c0b245-goog
From: Dmitry Torokhov <[email protected]>
In preparation for handling embedded USB devices let's split
usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and
usb_acpi_find_companion_for_port().
Signed-off-by: Dmitry Torokhov <[email protected]>
Signed-off-by: Rajat Jain <[email protected]>
---
v3: same as v1
v2: same as v1
drivers/usb/core/usb-acpi.c | 133 +++++++++++++++++++-----------------
1 file changed, 72 insertions(+), 61 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index e221861b3187..8ff73c83e8e8 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -139,12 +139,79 @@ static struct acpi_device *usb_acpi_find_port(struct acpi_device *parent,
return acpi_find_child_device(parent, raw, false);
}
-static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+static struct acpi_device *
+usb_acpi_get_companion_for_port(struct usb_port *port_dev)
{
struct usb_device *udev;
struct acpi_device *adev;
acpi_handle *parent_handle;
+ int port1;
+
+ /* Get the struct usb_device point of port's hub */
+ udev = to_usb_device(port_dev->dev.parent->parent);
+
+ /*
+ * The root hub ports' parent is the root hub. The non-root-hub
+ * ports' parent is the parent hub port which the hub is
+ * connected to.
+ */
+ if (!udev->parent) {
+ adev = ACPI_COMPANION(&udev->dev);
+ port1 = usb_hcd_find_raw_port_number(bus_to_hcd(udev->bus),
+ port_dev->portnum);
+ } else {
+ parent_handle = usb_get_hub_port_acpi_handle(udev->parent,
+ udev->portnum);
+ if (!parent_handle)
+ return NULL;
+
+ acpi_bus_get_device(parent_handle, &adev);
+ port1 = port_dev->portnum;
+ }
+
+ return usb_acpi_find_port(adev, port1);
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_port(struct usb_port *port_dev)
+{
+ struct acpi_device *adev;
+ struct acpi_pld_info *pld;
+ acpi_handle *handle;
+ acpi_status status;
+
+ adev = usb_acpi_get_companion_for_port(port_dev);
+ if (!adev)
+ return NULL;
+
+ handle = adev->handle;
+ status = acpi_get_physical_device_location(handle, &pld);
+ if (!ACPI_FAILURE(status) && pld) {
+ port_dev->location = USB_ACPI_LOCATION_VALID
+ | pld->group_token << 8 | pld->group_position;
+ port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
+ ACPI_FREE(pld);
+ }
+ return adev;
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_device(struct usb_device *udev)
+{
+ struct acpi_device *adev;
+
+ if (!udev->parent)
+ return NULL;
+
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+}
+
+
+static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+{
/*
* In the ACPI DSDT table, only usb root hub and usb ports are
* acpi device nodes. The hierarchy like following.
@@ -158,66 +225,10 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
* So all binding process is divided into two parts. binding
* root hub and usb ports.
*/
- if (is_usb_device(dev)) {
- udev = to_usb_device(dev);
- if (udev->parent)
- return NULL;
-
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(dev->parent);
- return acpi_find_child_device(adev, 0, false);
- } else if (is_usb_port(dev)) {
- struct usb_port *port_dev = to_usb_port(dev);
- int port1 = port_dev->portnum;
- struct acpi_pld_info *pld;
- acpi_handle *handle;
- acpi_status status;
-
- /* Get the struct usb_device point of port's hub */
- udev = to_usb_device(dev->parent->parent);
-
- /*
- * The root hub ports' parent is the root hub. The non-root-hub
- * ports' parent is the parent hub port which the hub is
- * connected to.
- */
- if (!udev->parent) {
- struct usb_hcd *hcd = bus_to_hcd(udev->bus);
- int raw;
-
- raw = usb_hcd_find_raw_port_number(hcd, port1);
-
- adev = usb_acpi_find_port(ACPI_COMPANION(&udev->dev),
- raw);
-
- if (!adev)
- return NULL;
- } else {
- parent_handle =
- usb_get_hub_port_acpi_handle(udev->parent,
- udev->portnum);
- if (!parent_handle)
- return NULL;
-
- acpi_bus_get_device(parent_handle, &adev);
-
- adev = usb_acpi_find_port(adev, port1);
-
- if (!adev)
- return NULL;
- }
- handle = adev->handle;
- status = acpi_get_physical_device_location(handle, &pld);
- if (ACPI_FAILURE(status) || !pld)
- return adev;
-
- port_dev->location = USB_ACPI_LOCATION_VALID
- | pld->group_token << 8 | pld->group_position;
- port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
- ACPI_FREE(pld);
-
- return adev;
- }
+ if (is_usb_device(dev))
+ return usb_acpi_find_companion_for_device(to_usb_device(dev));
+ else if (is_usb_port(dev))
+ return usb_acpi_find_companion_for_port(to_usb_port(dev));
return NULL;
}
--
2.19.1.1215.g8438c0b245-goog
From: Dmitry Torokhov <[email protected]>
USB devices permanently connected to USB ports may be described in ACPI
tables and share ACPI devices with ports they are connected to. See [1]
for details.
This will allow us to describe sideband resources for devices, such as,
for example, hard reset line for BT USB controllers.
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects#acpi-namespace-hierarchy-and-adr-for-embedded-usb-devices
Signed-off-by: Dmitry Torokhov <[email protected]>
Signed-off-by: Rajat Jain <[email protected]> (changed how we get the usb_port)
---
v3: same as v1
v2: same as v1
drivers/usb/core/usb-acpi.c | 44 +++++++++++++++++++++++++++++--------
1 file changed, 35 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 8ff73c83e8e8..9043d7242d67 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -200,30 +200,56 @@ static struct acpi_device *
usb_acpi_find_companion_for_device(struct usb_device *udev)
{
struct acpi_device *adev;
+ struct usb_port *port_dev;
+ struct usb_hub *hub;
+
+ if (!udev->parent) {
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+ }
- if (!udev->parent)
+ hub = usb_hub_to_struct_hub(udev->parent);
+ if (!hub)
return NULL;
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(udev->dev.parent);
- return acpi_find_child_device(adev, 0, false);
+ /*
+ * This is an embedded USB device connected to a port and such
+ * devices share port's ACPI companion.
+ */
+ port_dev = hub->ports[udev->portnum - 1];
+ return usb_acpi_get_companion_for_port(port_dev);
}
-
static struct acpi_device *usb_acpi_find_companion(struct device *dev)
{
/*
- * In the ACPI DSDT table, only usb root hub and usb ports are
- * acpi device nodes. The hierarchy like following.
+ * The USB hierarchy like following:
+ *
* Device (EHC1)
* Device (HUBN)
* Device (PR01)
* Device (PR11)
* Device (PR12)
+ * Device (FN12)
+ * Device (FN13)
* Device (PR13)
* ...
- * So all binding process is divided into two parts. binding
- * root hub and usb ports.
+ * where HUBN is root hub, and PRNN are USB ports and devices
+ * connected to them, and FNNN are individualk functions for
+ * connected composite USB devices. PRNN and FNNN may contain
+ * _CRS and other methods describing sideband resources for
+ * the connected device.
+ *
+ * On the kernel side both root hub and embedded USB devices are
+ * represented as instances of usb_device structure, and ports
+ * are represented as usb_port structures, so the whole process
+ * is split into 2 parts: finding companions for devices and
+ * finding companions for ports.
+ *
+ * Note that we do not handle individual functions of composite
+ * devices yet, for that we would need to assign companions to
+ * devices corresponding to USB interfaces.
*/
if (is_usb_device(dev))
return usb_acpi_find_companion_for_device(to_usb_device(dev));
--
2.19.1.1215.g8438c0b245-goog
Add a quirk and a hook to allow the HCI core to reset the BT chip
if needed (after a number of timed out commands). Use that new hook to
initiate BT chip reset if the controller fails to respond to certain
number of commands (currently 5) including the HCI reset commands.
This is done based on a newly introduced quirk. This is done based
on some initial work by Intel.
Signed-off-by: Rajat Jain <[email protected]>
---
v3: same as v1
v2: same as v1
include/net/bluetooth/hci.h | 8 ++++++++
include/net/bluetooth/hci_core.h | 2 ++
net/bluetooth/hci_core.c | 15 +++++++++++++--
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c36dc1e20556..af02fa5ffe54 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -192,6 +192,14 @@ enum {
*
*/
HCI_QUIRK_NON_PERSISTENT_SETUP,
+
+ /* When this quirk is set, hw_reset() would be run to reset the
+ * hardware, after a certain number of commands (currently 5)
+ * time out because the device fails to respond.
+ *
+ * This quirk should be set before hci_register_dev is called.
+ */
+ HCI_QUIRK_HW_RESET_ON_TIMEOUT,
};
/* HCI device flags */
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e5ea633ea368..b86218304b80 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -313,6 +313,7 @@ struct hci_dev {
unsigned int acl_cnt;
unsigned int sco_cnt;
unsigned int le_cnt;
+ unsigned int timeout_cnt;
unsigned int acl_mtu;
unsigned int sco_mtu;
@@ -437,6 +438,7 @@ struct hci_dev {
int (*post_init)(struct hci_dev *hdev);
int (*set_diag)(struct hci_dev *hdev, bool enable);
int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
+ void (*hw_reset)(struct hci_dev *hdev);
};
#define HCI_PHY_HANDLE(handle) (handle & 0xff)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7352fe85674b..ab3a6a8b7ba6 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2569,13 +2569,24 @@ static void hci_cmd_timeout(struct work_struct *work)
struct hci_dev *hdev = container_of(work, struct hci_dev,
cmd_timer.work);
+ hdev->timeout_cnt++;
if (hdev->sent_cmd) {
struct hci_command_hdr *sent = (void *) hdev->sent_cmd->data;
u16 opcode = __le16_to_cpu(sent->opcode);
- bt_dev_err(hdev, "command 0x%4.4x tx timeout", opcode);
+ bt_dev_err(hdev, "command 0x%4.4x tx timeout (cnt = %u)",
+ opcode, hdev->timeout_cnt);
} else {
- bt_dev_err(hdev, "command tx timeout");
+ bt_dev_err(hdev, "command tx timeout (cnt = %u)",
+ hdev->timeout_cnt);
+ }
+
+ if (test_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks) &&
+ hdev->timeout_cnt >= 5) {
+ hdev->timeout_cnt = 0;
+ if (hdev->hw_reset)
+ hdev->hw_reset(hdev);
+ return;
}
atomic_set(&hdev->cmd_cnt, 1);
--
2.19.1.1215.g8438c0b245-goog
The BTUSB_INTEL and BTUSB_INTEL_NEW have common functions & quirks are
assigned to hdev structure. Lets collect them together instead of
repeating them in different code branches.
Signed-off-by: Rajat Jain <[email protected]>
---
v3: same as v1
v2: same as v1
drivers/bluetooth/btusb.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 7439a7eb50ac..e8e148480c91 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3077,28 +3077,25 @@ static int btusb_probe(struct usb_interface *intf,
data->diag = usb_ifnum_to_if(data->udev, ifnum_base + 2);
}
#endif
+ if (id->driver_info & BTUSB_INTEL ||
+ id->driver_info & BTUSB_INTEL_NEW) {
- if (id->driver_info & BTUSB_INTEL) {
hdev->manufacturer = 2;
- hdev->setup = btusb_setup_intel;
- hdev->shutdown = btusb_shutdown_intel;
- hdev->set_diag = btintel_set_diag_mfg;
hdev->set_bdaddr = btintel_set_bdaddr;
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 (id->driver_info & BTUSB_INTEL_NEW) {
- hdev->manufacturer = 2;
- hdev->send = btusb_send_frame_intel;
- hdev->setup = btusb_setup_intel_new;
- hdev->hw_error = btintel_hw_error;
- hdev->set_diag = btintel_set_diag;
- hdev->set_bdaddr = btintel_set_bdaddr;
- 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 (id->driver_info & BTUSB_INTEL) {
+ hdev->setup = btusb_setup_intel;
+ hdev->shutdown = btusb_shutdown_intel;
+ hdev->set_diag = btintel_set_diag_mfg;
+ } else {
+ hdev->send = btusb_send_frame_intel;
+ hdev->setup = btusb_setup_intel_new;
+ hdev->hw_error = btintel_hw_error;
+ hdev->set_diag = btintel_set_diag;
+ }
}
if (id->driver_info & BTUSB_MARVELL)
--
2.19.1.1215.g8438c0b245-goog
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 <[email protected]>
---
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
On Wed, Nov 21, 2018 at 03:50:17PM -0800, Rajat Jain wrote:
> From: Dmitry Torokhov <[email protected]>
>
> USB devices permanently connected to USB ports may be described in ACPI
> tables and share ACPI devices with ports they are connected to. See [1]
> for details.
>
> This will allow us to describe sideband resources for devices, such as,
> for example, hard reset line for BT USB controllers.
>
> [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects#acpi-namespace-hierarchy-and-adr-for-embedded-usb-devices
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> Signed-off-by: Rajat Jain <[email protected]> (changed how we get the usb_port)
Acked-by: Greg Kroah-Hartman <[email protected]>
On Wed, Nov 21, 2018 at 03:50:16PM -0800, Rajat Jain wrote:
> From: Dmitry Torokhov <[email protected]>
>
> In preparation for handling embedded USB devices let's split
> usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and
> usb_acpi_find_companion_for_port().
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> Signed-off-by: Rajat Jain <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
>On Wed, Nov 21, 2018 at 03:50:17PM -0800, Rajat Jain wrote:
>> From: Dmitry Torokhov <[email protected]>
>>
>> USB devices permanently connected to USB ports may be described in
>> ACPI tables and share ACPI devices with ports they are connected to.
>> See [1] for details.
>>
>> This will allow us to describe sideband resources for devices, such
>> as, for example, hard reset line for BT USB controllers.
>>
>> [1]
>> https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/othe
>> r-acpi-namespace-objects#acpi-namespace-hierarchy-and-adr-for-
>embedded
>> -usb-devices
>>
>> Signed-off-by: Dmitry Torokhov <[email protected]>
>> Signed-off-by: Rajat Jain <[email protected]> (changed how we get the
>> usb_port)
>
>Acked-by: Greg Kroah-Hartman <[email protected]>
Tested-by: Sukumar Ghorai <[email protected]>
>On Wed, Nov 21, 2018 at 03:50:16PM -0800, Rajat Jain wrote:
>> From: Dmitry Torokhov <[email protected]>
>>
>> In preparation for handling embedded USB devices let's split
>> usb_acpi_find_companion() into usb_acpi_find_companion_for_device()
>> and usb_acpi_find_companion_for_port().
>>
>> Signed-off-by: Dmitry Torokhov <[email protected]>
>> Signed-off-by: Rajat Jain <[email protected]>
>
>Acked-by: Greg Kroah-Hartman <[email protected]>
Tested-by: Sukumar Ghorai <[email protected]>
On Wed, Nov 21, 2018 at 3:50 PM Rajat Jain <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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
>
From: Dmitry Torokhov <[email protected]>
In preparation for handling embedded USB devices let's split
usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and
usb_acpi_find_companion_for_port().
Signed-off-by: Dmitry Torokhov <[email protected]>
Signed-off-by: Rajat Jain <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Tested-by: Sukumar Ghorai <[email protected]>
---
v4: Add Acked-by and Tested-by in signatures.
v3: same as v1
v2: same as v1
drivers/usb/core/usb-acpi.c | 133 +++++++++++++++++++-----------------
1 file changed, 72 insertions(+), 61 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index e221861b3187..8ff73c83e8e8 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -139,12 +139,79 @@ static struct acpi_device *usb_acpi_find_port(struct acpi_device *parent,
return acpi_find_child_device(parent, raw, false);
}
-static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+static struct acpi_device *
+usb_acpi_get_companion_for_port(struct usb_port *port_dev)
{
struct usb_device *udev;
struct acpi_device *adev;
acpi_handle *parent_handle;
+ int port1;
+
+ /* Get the struct usb_device point of port's hub */
+ udev = to_usb_device(port_dev->dev.parent->parent);
+
+ /*
+ * The root hub ports' parent is the root hub. The non-root-hub
+ * ports' parent is the parent hub port which the hub is
+ * connected to.
+ */
+ if (!udev->parent) {
+ adev = ACPI_COMPANION(&udev->dev);
+ port1 = usb_hcd_find_raw_port_number(bus_to_hcd(udev->bus),
+ port_dev->portnum);
+ } else {
+ parent_handle = usb_get_hub_port_acpi_handle(udev->parent,
+ udev->portnum);
+ if (!parent_handle)
+ return NULL;
+
+ acpi_bus_get_device(parent_handle, &adev);
+ port1 = port_dev->portnum;
+ }
+
+ return usb_acpi_find_port(adev, port1);
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_port(struct usb_port *port_dev)
+{
+ struct acpi_device *adev;
+ struct acpi_pld_info *pld;
+ acpi_handle *handle;
+ acpi_status status;
+
+ adev = usb_acpi_get_companion_for_port(port_dev);
+ if (!adev)
+ return NULL;
+
+ handle = adev->handle;
+ status = acpi_get_physical_device_location(handle, &pld);
+ if (!ACPI_FAILURE(status) && pld) {
+ port_dev->location = USB_ACPI_LOCATION_VALID
+ | pld->group_token << 8 | pld->group_position;
+ port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
+ ACPI_FREE(pld);
+ }
+ return adev;
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_device(struct usb_device *udev)
+{
+ struct acpi_device *adev;
+
+ if (!udev->parent)
+ return NULL;
+
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+}
+
+
+static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+{
/*
* In the ACPI DSDT table, only usb root hub and usb ports are
* acpi device nodes. The hierarchy like following.
@@ -158,66 +225,10 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
* So all binding process is divided into two parts. binding
* root hub and usb ports.
*/
- if (is_usb_device(dev)) {
- udev = to_usb_device(dev);
- if (udev->parent)
- return NULL;
-
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(dev->parent);
- return acpi_find_child_device(adev, 0, false);
- } else if (is_usb_port(dev)) {
- struct usb_port *port_dev = to_usb_port(dev);
- int port1 = port_dev->portnum;
- struct acpi_pld_info *pld;
- acpi_handle *handle;
- acpi_status status;
-
- /* Get the struct usb_device point of port's hub */
- udev = to_usb_device(dev->parent->parent);
-
- /*
- * The root hub ports' parent is the root hub. The non-root-hub
- * ports' parent is the parent hub port which the hub is
- * connected to.
- */
- if (!udev->parent) {
- struct usb_hcd *hcd = bus_to_hcd(udev->bus);
- int raw;
-
- raw = usb_hcd_find_raw_port_number(hcd, port1);
-
- adev = usb_acpi_find_port(ACPI_COMPANION(&udev->dev),
- raw);
-
- if (!adev)
- return NULL;
- } else {
- parent_handle =
- usb_get_hub_port_acpi_handle(udev->parent,
- udev->portnum);
- if (!parent_handle)
- return NULL;
-
- acpi_bus_get_device(parent_handle, &adev);
-
- adev = usb_acpi_find_port(adev, port1);
-
- if (!adev)
- return NULL;
- }
- handle = adev->handle;
- status = acpi_get_physical_device_location(handle, &pld);
- if (ACPI_FAILURE(status) || !pld)
- return adev;
-
- port_dev->location = USB_ACPI_LOCATION_VALID
- | pld->group_token << 8 | pld->group_position;
- port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
- ACPI_FREE(pld);
-
- return adev;
- }
+ if (is_usb_device(dev))
+ return usb_acpi_find_companion_for_device(to_usb_device(dev));
+ else if (is_usb_port(dev))
+ return usb_acpi_find_companion_for_port(to_usb_port(dev));
return NULL;
}
--
2.20.1.321.g9e740568ce-goog
Add a quirk and a hook to allow the HCI core to reset the BT chip
if needed (after a number of timed out commands). Use that new hook to
initiate BT chip reset if the controller fails to respond to certain
number of commands (currently 5) including the HCI reset commands.
This is done based on a newly introduced quirk. This is done based
on some initial work by Intel.
Signed-off-by: Rajat Jain <[email protected]>
---
v4: same as v1
v3: same as v1
v2: same as v1
include/net/bluetooth/hci.h | 8 ++++++++
include/net/bluetooth/hci_core.h | 2 ++
net/bluetooth/hci_core.c | 15 +++++++++++++--
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c36dc1e20556..af02fa5ffe54 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -192,6 +192,14 @@ enum {
*
*/
HCI_QUIRK_NON_PERSISTENT_SETUP,
+
+ /* When this quirk is set, hw_reset() would be run to reset the
+ * hardware, after a certain number of commands (currently 5)
+ * time out because the device fails to respond.
+ *
+ * This quirk should be set before hci_register_dev is called.
+ */
+ HCI_QUIRK_HW_RESET_ON_TIMEOUT,
};
/* HCI device flags */
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e5ea633ea368..b86218304b80 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -313,6 +313,7 @@ struct hci_dev {
unsigned int acl_cnt;
unsigned int sco_cnt;
unsigned int le_cnt;
+ unsigned int timeout_cnt;
unsigned int acl_mtu;
unsigned int sco_mtu;
@@ -437,6 +438,7 @@ struct hci_dev {
int (*post_init)(struct hci_dev *hdev);
int (*set_diag)(struct hci_dev *hdev, bool enable);
int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
+ void (*hw_reset)(struct hci_dev *hdev);
};
#define HCI_PHY_HANDLE(handle) (handle & 0xff)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7352fe85674b..ab3a6a8b7ba6 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2569,13 +2569,24 @@ static void hci_cmd_timeout(struct work_struct *work)
struct hci_dev *hdev = container_of(work, struct hci_dev,
cmd_timer.work);
+ hdev->timeout_cnt++;
if (hdev->sent_cmd) {
struct hci_command_hdr *sent = (void *) hdev->sent_cmd->data;
u16 opcode = __le16_to_cpu(sent->opcode);
- bt_dev_err(hdev, "command 0x%4.4x tx timeout", opcode);
+ bt_dev_err(hdev, "command 0x%4.4x tx timeout (cnt = %u)",
+ opcode, hdev->timeout_cnt);
} else {
- bt_dev_err(hdev, "command tx timeout");
+ bt_dev_err(hdev, "command tx timeout (cnt = %u)",
+ hdev->timeout_cnt);
+ }
+
+ if (test_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks) &&
+ hdev->timeout_cnt >= 5) {
+ hdev->timeout_cnt = 0;
+ if (hdev->hw_reset)
+ hdev->hw_reset(hdev);
+ return;
}
atomic_set(&hdev->cmd_cnt, 1);
--
2.20.1.321.g9e740568ce-goog
From: Dmitry Torokhov <[email protected]>
USB devices permanently connected to USB ports may be described in ACPI
tables and share ACPI devices with ports they are connected to. See [1]
for details.
This will allow us to describe sideband resources for devices, such as,
for example, hard reset line for BT USB controllers.
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects#acpi-namespace-hierarchy-and-adr-for-embedded-usb-devices
Signed-off-by: Dmitry Torokhov <[email protected]>
Signed-off-by: Rajat Jain <[email protected]> (changed how we get the usb_port)
Acked-by: Greg Kroah-Hartman <[email protected]>
Tested-by: Sukumar Ghorai <[email protected]>
---
v4: Add Acked-by and Tested-by in signatures.
v3: same as v1
v2: same as v1
drivers/usb/core/usb-acpi.c | 44 +++++++++++++++++++++++++++++--------
1 file changed, 35 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 8ff73c83e8e8..9043d7242d67 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -200,30 +200,56 @@ static struct acpi_device *
usb_acpi_find_companion_for_device(struct usb_device *udev)
{
struct acpi_device *adev;
+ struct usb_port *port_dev;
+ struct usb_hub *hub;
+
+ if (!udev->parent) {
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+ }
- if (!udev->parent)
+ hub = usb_hub_to_struct_hub(udev->parent);
+ if (!hub)
return NULL;
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(udev->dev.parent);
- return acpi_find_child_device(adev, 0, false);
+ /*
+ * This is an embedded USB device connected to a port and such
+ * devices share port's ACPI companion.
+ */
+ port_dev = hub->ports[udev->portnum - 1];
+ return usb_acpi_get_companion_for_port(port_dev);
}
-
static struct acpi_device *usb_acpi_find_companion(struct device *dev)
{
/*
- * In the ACPI DSDT table, only usb root hub and usb ports are
- * acpi device nodes. The hierarchy like following.
+ * The USB hierarchy like following:
+ *
* Device (EHC1)
* Device (HUBN)
* Device (PR01)
* Device (PR11)
* Device (PR12)
+ * Device (FN12)
+ * Device (FN13)
* Device (PR13)
* ...
- * So all binding process is divided into two parts. binding
- * root hub and usb ports.
+ * where HUBN is root hub, and PRNN are USB ports and devices
+ * connected to them, and FNNN are individualk functions for
+ * connected composite USB devices. PRNN and FNNN may contain
+ * _CRS and other methods describing sideband resources for
+ * the connected device.
+ *
+ * On the kernel side both root hub and embedded USB devices are
+ * represented as instances of usb_device structure, and ports
+ * are represented as usb_port structures, so the whole process
+ * is split into 2 parts: finding companions for devices and
+ * finding companions for ports.
+ *
+ * Note that we do not handle individual functions of composite
+ * devices yet, for that we would need to assign companions to
+ * devices corresponding to USB interfaces.
*/
if (is_usb_device(dev))
return usb_acpi_find_companion_for_device(to_usb_device(dev));
--
2.20.1.321.g9e740568ce-goog
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 <[email protected]>
---
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 | 45 +++++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 59869643cc29..7cf1e4f749e9 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_DONE 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;
@@ -491,6 +495,30 @@ 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.
+ */
+ if (test_and_set_bit(BTUSB_HW_RESET_DONE, &data->flags)) {
+ bt_dev_warn(hdev, "last reset failed? Not resetting again\n");
+ return;
+ }
+
+ bt_dev_dbg(hdev, "Initiating HW reset via gpio\n");
+ 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 +2943,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 +3057,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;
@@ -3083,6 +3122,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;
@@ -3223,6 +3263,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;
}
@@ -3266,6 +3308,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.20.1.321.g9e740568ce-goog
The BTUSB_INTEL and BTUSB_INTEL_NEW have common functions & quirks are
assigned to hdev structure. Lets collect them together instead of
repeating them in different code branches.
Signed-off-by: Rajat Jain <[email protected]>
---
v4: same as v1
v3: same as v1
v2: same as v1
drivers/bluetooth/btusb.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 4761499db9ee..59869643cc29 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3075,28 +3075,25 @@ static int btusb_probe(struct usb_interface *intf,
data->diag = usb_ifnum_to_if(data->udev, ifnum_base + 2);
}
#endif
+ if (id->driver_info & BTUSB_INTEL ||
+ id->driver_info & BTUSB_INTEL_NEW) {
- if (id->driver_info & BTUSB_INTEL) {
hdev->manufacturer = 2;
- hdev->setup = btusb_setup_intel;
- hdev->shutdown = btusb_shutdown_intel;
- hdev->set_diag = btintel_set_diag_mfg;
hdev->set_bdaddr = btintel_set_bdaddr;
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 (id->driver_info & BTUSB_INTEL_NEW) {
- hdev->manufacturer = 2;
- hdev->send = btusb_send_frame_intel;
- hdev->setup = btusb_setup_intel_new;
- hdev->hw_error = btintel_hw_error;
- hdev->set_diag = btintel_set_diag;
- hdev->set_bdaddr = btintel_set_bdaddr;
- 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 (id->driver_info & BTUSB_INTEL) {
+ hdev->setup = btusb_setup_intel;
+ hdev->shutdown = btusb_shutdown_intel;
+ hdev->set_diag = btintel_set_diag_mfg;
+ } else {
+ hdev->send = btusb_send_frame_intel;
+ hdev->setup = btusb_setup_intel_new;
+ hdev->hw_error = btintel_hw_error;
+ hdev->set_diag = btintel_set_diag;
+ }
}
if (id->driver_info & BTUSB_MARVELL)
--
2.20.1.321.g9e740568ce-goog
Hi Rajat,
> USB devices permanently connected to USB ports may be described in ACPI
> tables and share ACPI devices with ports they are connected to. See [1]
> for details.
>
> This will allow us to describe sideband resources for devices, such as,
> for example, hard reset line for BT USB controllers.
>
> [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects#acpi-namespace-hierarchy-and-adr-for-embedded-usb-devices
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> Signed-off-by: Rajat Jain <[email protected]> (changed how we get the usb_port)
> Acked-by: Greg Kroah-Hartman <[email protected]>
> Tested-by: Sukumar Ghorai <[email protected]>
> ---
> v4: Add Acked-by and Tested-by in signatures.
> v3: same as v1
> v2: same as v1
>
> drivers/usb/core/usb-acpi.c | 44 +++++++++++++++++++++++++++++--------
> 1 file changed, 35 insertions(+), 9 deletions(-)
same question here. Which tree is suppose to take this?
Regards
Marcel
Hi Rajat,
> Add a quirk and a hook to allow the HCI core to reset the BT chip
> if needed (after a number of timed out commands). Use that new hook to
> initiate BT chip reset if the controller fails to respond to certain
> number of commands (currently 5) including the HCI reset commands.
> This is done based on a newly introduced quirk. This is done based
> on some initial work by Intel.
>
> Signed-off-by: Rajat Jain <[email protected]>
> ---
> v4: same as v1
> v3: same as v1
> v2: same as v1
>
> include/net/bluetooth/hci.h | 8 ++++++++
> include/net/bluetooth/hci_core.h | 2 ++
> net/bluetooth/hci_core.c | 15 +++++++++++++--
> 3 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index c36dc1e20556..af02fa5ffe54 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -192,6 +192,14 @@ enum {
> *
> */
> HCI_QUIRK_NON_PERSISTENT_SETUP,
> +
> + /* When this quirk is set, hw_reset() would be run to reset the
> + * hardware, after a certain number of commands (currently 5)
> + * time out because the device fails to respond.
> + *
> + * This quirk should be set before hci_register_dev is called.
> + */
> + HCI_QUIRK_HW_RESET_ON_TIMEOUT,
> };
>
> /* HCI device flags */
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index e5ea633ea368..b86218304b80 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -313,6 +313,7 @@ struct hci_dev {
> unsigned int acl_cnt;
> unsigned int sco_cnt;
> unsigned int le_cnt;
> + unsigned int timeout_cnt;
>
> unsigned int acl_mtu;
> unsigned int sco_mtu;
> @@ -437,6 +438,7 @@ struct hci_dev {
> int (*post_init)(struct hci_dev *hdev);
> int (*set_diag)(struct hci_dev *hdev, bool enable);
> int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> + void (*hw_reset)(struct hci_dev *hdev);
> };
>
> #define HCI_PHY_HANDLE(handle) (handle & 0xff)
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 7352fe85674b..ab3a6a8b7ba6 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2569,13 +2569,24 @@ static void hci_cmd_timeout(struct work_struct *work)
> struct hci_dev *hdev = container_of(work, struct hci_dev,
> cmd_timer.work);
>
> + hdev->timeout_cnt++;
> if (hdev->sent_cmd) {
> struct hci_command_hdr *sent = (void *) hdev->sent_cmd->data;
> u16 opcode = __le16_to_cpu(sent->opcode);
>
> - bt_dev_err(hdev, "command 0x%4.4x tx timeout", opcode);
> + bt_dev_err(hdev, "command 0x%4.4x tx timeout (cnt = %u)",
> + opcode, hdev->timeout_cnt);
> } else {
> - bt_dev_err(hdev, "command tx timeout");
> + bt_dev_err(hdev, "command tx timeout (cnt = %u)",
> + hdev->timeout_cnt);
> + }
> +
> + if (test_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks) &&
> + hdev->timeout_cnt >= 5) {
> + hdev->timeout_cnt = 0;
> + if (hdev->hw_reset)
> + hdev->hw_reset(hdev);
> + return;
> }
so I really do not see the need for the quirk here. Either hdev->hw_reset is provided, then execute it, if it is not provided then don’t. The quirk is just duplicate information.
I also don’t like hdev->hw_reset since that implies that the only way of handling a command timeout is a hardware reset. I prefer you call this hdev->cmd_timeout and also scrap the timeout_cnt. Let the driver decide what number of timeouts it wants to react on. The number 5 is just an arbitrary number you picked based on one hardware manufacturer.
Regards
Marcel
Hi Rajat,
> The BTUSB_INTEL and BTUSB_INTEL_NEW have common functions & quirks are
> assigned to hdev structure. Lets collect them together instead of
> repeating them in different code branches.
>
> Signed-off-by: Rajat Jain <[email protected]>
> ---
> v4: same as v1
> v3: same as v1
> v2: same as v1
>
> drivers/bluetooth/btusb.c | 27 ++++++++++++---------------
> 1 file changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 4761499db9ee..59869643cc29 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -3075,28 +3075,25 @@ static int btusb_probe(struct usb_interface *intf,
> data->diag = usb_ifnum_to_if(data->udev, ifnum_base + 2);
> }
> #endif
> + if (id->driver_info & BTUSB_INTEL ||
> + id->driver_info & BTUSB_INTEL_NEW) {
>
> - if (id->driver_info & BTUSB_INTEL) {
> hdev->manufacturer = 2;
> - hdev->setup = btusb_setup_intel;
> - hdev->shutdown = btusb_shutdown_intel;
> - hdev->set_diag = btintel_set_diag_mfg;
> hdev->set_bdaddr = btintel_set_bdaddr;
> 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 (id->driver_info & BTUSB_INTEL_NEW) {
> - hdev->manufacturer = 2;
> - hdev->send = btusb_send_frame_intel;
> - hdev->setup = btusb_setup_intel_new;
> - hdev->hw_error = btintel_hw_error;
> - hdev->set_diag = btintel_set_diag;
> - hdev->set_bdaddr = btintel_set_bdaddr;
> - 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 (id->driver_info & BTUSB_INTEL) {
> + hdev->setup = btusb_setup_intel;
> + hdev->shutdown = btusb_shutdown_intel;
> + hdev->set_diag = btintel_set_diag_mfg;
> + } else {
> + hdev->send = btusb_send_frame_intel;
> + hdev->setup = btusb_setup_intel_new;
> + hdev->hw_error = btintel_hw_error;
> + hdev->set_diag = btintel_set_diag;
> + }
> }
please scrap this patch since it is not making anything easier or simpler. You think it does, but it really doesn’t.
Regards
Marcel
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 <[email protected]>
> ---
> 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 | 45 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 59869643cc29..7cf1e4f749e9 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_DONE 12
I think you mean BTUSB_HW_RESET_ACTIVE or BTUSB_HW_RESET_IN_PROGRESS.
>
> 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;
>
> @@ -491,6 +495,30 @@ 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.
> + */
> + if (test_and_set_bit(BTUSB_HW_RESET_DONE, &data->flags)) {
> + bt_dev_warn(hdev, "last reset failed? Not resetting again\n");
> + return;
> + }
> +
> + bt_dev_dbg(hdev, "Initiating HW reset via gpio\n”);
The bt_dev_ functions don’t need the \n at the end.
> + 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 +2943,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 +3057,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;
> @@ -3083,6 +3122,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;
> @@ -3223,6 +3263,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;
> }
> @@ -3266,6 +3308,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 Rajat,
> In preparation for handling embedded USB devices let's split
> usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and
> usb_acpi_find_companion_for_port().
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> Signed-off-by: Rajat Jain <[email protected]>
> Acked-by: Greg Kroah-Hartman <[email protected]>
> Tested-by: Sukumar Ghorai <[email protected]>
> ---
> v4: Add Acked-by and Tested-by in signatures.
> v3: same as v1
> v2: same as v1
>
> drivers/usb/core/usb-acpi.c | 133 +++++++++++++++++++-----------------
> 1 file changed, 72 insertions(+), 61 deletions(-)
what is the plan here? I take this via bluetooth-next tree?
Regards
Marcel
Hi Marcel,
On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <[email protected]> wrote:
>
> Hi Rajat,
>
> > In preparation for handling embedded USB devices let's split
> > usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and
> > usb_acpi_find_companion_for_port().
> >
> > Signed-off-by: Dmitry Torokhov <[email protected]>
> > Signed-off-by: Rajat Jain <[email protected]>
> > Acked-by: Greg Kroah-Hartman <[email protected]>
> > Tested-by: Sukumar Ghorai <[email protected]>
> > ---
> > v4: Add Acked-by and Tested-by in signatures.
> > v3: same as v1
> > v2: same as v1
> >
> > drivers/usb/core/usb-acpi.c | 133 +++++++++++++++++++-----------------
> > 1 file changed, 72 insertions(+), 61 deletions(-)
>
> what is the plan here? I take this via bluetooth-next tree?
Yes, I'd think that would be the best plan. Dmitry / Greg - do you
have any objections / suggestions?
>
> Regards
>
> Marcel
>
On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <[email protected]> wrote:
>
> Hi Rajat,
>
> > USB devices permanently connected to USB ports may be described in ACPI
> > tables and share ACPI devices with ports they are connected to. See [1]
> > for details.
> >
> > This will allow us to describe sideband resources for devices, such as,
> > for example, hard reset line for BT USB controllers.
> >
> > [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects#acpi-namespace-hierarchy-and-adr-for-embedded-usb-devices
> >
> > Signed-off-by: Dmitry Torokhov <[email protected]>
> > Signed-off-by: Rajat Jain <[email protected]> (changed how we get the usb_port)
> > Acked-by: Greg Kroah-Hartman <[email protected]>
> > Tested-by: Sukumar Ghorai <[email protected]>
> > ---
> > v4: Add Acked-by and Tested-by in signatures.
> > v3: same as v1
> > v2: same as v1
> >
> > drivers/usb/core/usb-acpi.c | 44 +++++++++++++++++++++++++++++--------
> > 1 file changed, 35 insertions(+), 9 deletions(-)
>
> same question here. Which tree is suppose to take this?
Just responded to the 1st patch. Since the subsequent patches use this
feature, may be best to push this via your tree. Greg or Dmitry can
suggest if they have any other suggestions.
>
> Regards
>
> Marcel
>
On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <[email protected]> wrote:
>
> Hi Rajat,
>
> > Add a quirk and a hook to allow the HCI core to reset the BT chip
> > if needed (after a number of timed out commands). Use that new hook to
> > initiate BT chip reset if the controller fails to respond to certain
> > number of commands (currently 5) including the HCI reset commands.
> > This is done based on a newly introduced quirk. This is done based
> > on some initial work by Intel.
> >
> > Signed-off-by: Rajat Jain <[email protected]>
> > ---
> > v4: same as v1
> > v3: same as v1
> > v2: same as v1
> >
> > include/net/bluetooth/hci.h | 8 ++++++++
> > include/net/bluetooth/hci_core.h | 2 ++
> > net/bluetooth/hci_core.c | 15 +++++++++++++--
> > 3 files changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index c36dc1e20556..af02fa5ffe54 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -192,6 +192,14 @@ enum {
> > *
> > */
> > HCI_QUIRK_NON_PERSISTENT_SETUP,
> > +
> > + /* When this quirk is set, hw_reset() would be run to reset the
> > + * hardware, after a certain number of commands (currently 5)
> > + * time out because the device fails to respond.
> > + *
> > + * This quirk should be set before hci_register_dev is called.
> > + */
> > + HCI_QUIRK_HW_RESET_ON_TIMEOUT,
> > };
> >
> > /* HCI device flags */
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index e5ea633ea368..b86218304b80 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -313,6 +313,7 @@ struct hci_dev {
> > unsigned int acl_cnt;
> > unsigned int sco_cnt;
> > unsigned int le_cnt;
> > + unsigned int timeout_cnt;
> >
> > unsigned int acl_mtu;
> > unsigned int sco_mtu;
> > @@ -437,6 +438,7 @@ struct hci_dev {
> > int (*post_init)(struct hci_dev *hdev);
> > int (*set_diag)(struct hci_dev *hdev, bool enable);
> > int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> > + void (*hw_reset)(struct hci_dev *hdev);
> > };
> >
> > #define HCI_PHY_HANDLE(handle) (handle & 0xff)
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 7352fe85674b..ab3a6a8b7ba6 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -2569,13 +2569,24 @@ static void hci_cmd_timeout(struct work_struct *work)
> > struct hci_dev *hdev = container_of(work, struct hci_dev,
> > cmd_timer.work);
> >
> > + hdev->timeout_cnt++;
> > if (hdev->sent_cmd) {
> > struct hci_command_hdr *sent = (void *) hdev->sent_cmd->data;
> > u16 opcode = __le16_to_cpu(sent->opcode);
> >
> > - bt_dev_err(hdev, "command 0x%4.4x tx timeout", opcode);
> > + bt_dev_err(hdev, "command 0x%4.4x tx timeout (cnt = %u)",
> > + opcode, hdev->timeout_cnt);
> > } else {
> > - bt_dev_err(hdev, "command tx timeout");
> > + bt_dev_err(hdev, "command tx timeout (cnt = %u)",
> > + hdev->timeout_cnt);
> > + }
> > +
> > + if (test_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks) &&
> > + hdev->timeout_cnt >= 5) {
> > + hdev->timeout_cnt = 0;
> > + if (hdev->hw_reset)
> > + hdev->hw_reset(hdev);
> > + return;
> > }
>
> so I really do not see the need for the quirk here. Either hdev->hw_reset is provided, then execute it, if it is not provided then don’t. The quirk is just duplicate information.
Sure, will do.
>
> I also don’t like hdev->hw_reset since that implies that the only way of handling a command timeout is a hardware reset. I prefer you call this hdev->cmd_timeout and also scrap the timeout_cnt. Let the driver decide what number of timeouts it wants to react on. The number 5 is just an arbitrary number you picked based on one hardware manufacturer.
Sure, I can move the timeout_cnt from hdev to btusb_data so that the
btusb can track the number of the timeouts..
Thanks,
Rajat
>
> Regards
>
> Marcel
>
On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <[email protected]> wrote:
>
> Hi Rajat,
>
> > The BTUSB_INTEL and BTUSB_INTEL_NEW have common functions & quirks are
> > assigned to hdev structure. Lets collect them together instead of
> > repeating them in different code branches.
> >
> > Signed-off-by: Rajat Jain <[email protected]>
> > ---
> > v4: same as v1
> > v3: same as v1
> > v2: same as v1
> >
> > drivers/bluetooth/btusb.c | 27 ++++++++++++---------------
> > 1 file changed, 12 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 4761499db9ee..59869643cc29 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -3075,28 +3075,25 @@ static int btusb_probe(struct usb_interface *intf,
> > data->diag = usb_ifnum_to_if(data->udev, ifnum_base + 2);
> > }
> > #endif
> > + if (id->driver_info & BTUSB_INTEL ||
> > + id->driver_info & BTUSB_INTEL_NEW) {
> >
> > - if (id->driver_info & BTUSB_INTEL) {
> > hdev->manufacturer = 2;
> > - hdev->setup = btusb_setup_intel;
> > - hdev->shutdown = btusb_shutdown_intel;
> > - hdev->set_diag = btintel_set_diag_mfg;
> > hdev->set_bdaddr = btintel_set_bdaddr;
> > 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 (id->driver_info & BTUSB_INTEL_NEW) {
> > - hdev->manufacturer = 2;
> > - hdev->send = btusb_send_frame_intel;
> > - hdev->setup = btusb_setup_intel_new;
> > - hdev->hw_error = btintel_hw_error;
> > - hdev->set_diag = btintel_set_diag;
> > - hdev->set_bdaddr = btintel_set_bdaddr;
> > - 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 (id->driver_info & BTUSB_INTEL) {
> > + hdev->setup = btusb_setup_intel;
> > + hdev->shutdown = btusb_shutdown_intel;
> > + hdev->set_diag = btintel_set_diag_mfg;
> > + } else {
> > + hdev->send = btusb_send_frame_intel;
> > + hdev->setup = btusb_setup_intel_new;
> > + hdev->hw_error = btintel_hw_error;
> > + hdev->set_diag = btintel_set_diag;
> > + }
> > }
>
> please scrap this patch since it is not making anything easier or simpler. You think it does, but it really doesn’t.
OK, will do.
Thanks,
Rajat
>
> Regards
>
> Marcel
>
On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <[email protected]> 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 <[email protected]>
> > ---
> > 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 | 45 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 45 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 59869643cc29..7cf1e4f749e9 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_DONE 12
>
> I think you mean BTUSB_HW_RESET_ACTIVE or BTUSB_HW_RESET_IN_PROGRESS.
Sure, will do.
>
> >
> > 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;
> >
> > @@ -491,6 +495,30 @@ 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.
> > + */
> > + if (test_and_set_bit(BTUSB_HW_RESET_DONE, &data->flags)) {
> > + bt_dev_warn(hdev, "last reset failed? Not resetting again\n");
> > + return;
> > + }
> > +
> > + bt_dev_dbg(hdev, "Initiating HW reset via gpio\n”);
>
> The bt_dev_ functions don’t need the \n at the end.
Sure, will do.
>
> > + 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 +2943,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 +3057,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;
> > @@ -3083,6 +3122,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;
> > @@ -3223,6 +3263,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;
> > }
> > @@ -3266,6 +3308,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
>
Thanks,
Rajat
On Tue, Jan 22, 2019 at 2:29 PM Rajat Jain <[email protected]> wrote:
>
> Hi Marcel,
>
> On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <[email protected]> wrote:
> >
> > Hi Rajat,
> >
> > > In preparation for handling embedded USB devices let's split
> > > usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and
> > > usb_acpi_find_companion_for_port().
> > >
> > > Signed-off-by: Dmitry Torokhov <[email protected]>
> > > Signed-off-by: Rajat Jain <[email protected]>
> > > Acked-by: Greg Kroah-Hartman <[email protected]>
> > > Tested-by: Sukumar Ghorai <[email protected]>
> > > ---
> > > v4: Add Acked-by and Tested-by in signatures.
> > > v3: same as v1
> > > v2: same as v1
> > >
> > > drivers/usb/core/usb-acpi.c | 133 +++++++++++++++++++-----------------
> > > 1 file changed, 72 insertions(+), 61 deletions(-)
> >
> > what is the plan here? I take this via bluetooth-next tree?
>
> Yes, I'd think that would be the best plan. Dmitry / Greg - do you
> have any objections / suggestions?
That's up to Greg, but since he'd acked the patches I'd assume he's OK
with taking it through bluetooth. As an option Marcel could cut an
immutable branch off 4.20 with the first 2 patches so that he and Greg
can both pull it into their main branches and then git will do the
right thing when Linus pulls from them. Lee uses quite a bit of them
in MFD and other maintainers are known to occasionally make them for
work that needs to be shared between trees.
Thanks.
--
Dmitry
On Tue, Jan 22, 2019 at 02:42:26PM -0800, Dmitry Torokhov wrote:
> On Tue, Jan 22, 2019 at 2:29 PM Rajat Jain <[email protected]> wrote:
> >
> > Hi Marcel,
> >
> > On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <[email protected]> wrote:
> > >
> > > Hi Rajat,
> > >
> > > > In preparation for handling embedded USB devices let's split
> > > > usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and
> > > > usb_acpi_find_companion_for_port().
> > > >
> > > > Signed-off-by: Dmitry Torokhov <[email protected]>
> > > > Signed-off-by: Rajat Jain <[email protected]>
> > > > Acked-by: Greg Kroah-Hartman <[email protected]>
> > > > Tested-by: Sukumar Ghorai <[email protected]>
> > > > ---
> > > > v4: Add Acked-by and Tested-by in signatures.
> > > > v3: same as v1
> > > > v2: same as v1
> > > >
> > > > drivers/usb/core/usb-acpi.c | 133 +++++++++++++++++++-----------------
> > > > 1 file changed, 72 insertions(+), 61 deletions(-)
> > >
> > > what is the plan here? I take this via bluetooth-next tree?
> >
> > Yes, I'd think that would be the best plan. Dmitry / Greg - do you
> > have any objections / suggestions?
>
> That's up to Greg, but since he'd acked the patches I'd assume he's OK
> with taking it through bluetooth. As an option Marcel could cut an
> immutable branch off 4.20 with the first 2 patches so that he and Greg
> can both pull it into their main branches and then git will do the
> right thing when Linus pulls from them. Lee uses quite a bit of them
> in MFD and other maintainers are known to occasionally make them for
> work that needs to be shared between trees.
I don't need to take these, they can all go through one tree, which is
why I gave my ack to the patch.
thanks,
greg k-h
From: Dmitry Torokhov <[email protected]>
USB devices permanently connected to USB ports may be described in ACPI
tables and share ACPI devices with ports they are connected to. See [1]
for details.
This will allow us to describe sideband resources for devices, such as,
for example, hard reset line for BT USB controllers.
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects#acpi-namespace-hierarchy-and-adr-for-embedded-usb-devices
Signed-off-by: Dmitry Torokhov <[email protected]>
Signed-off-by: Rajat Jain <[email protected]> (changed how we get the usb_port)
Acked-by: Greg Kroah-Hartman <[email protected]>
Tested-by: Sukumar Ghorai <[email protected]>
---
v5: same as v4
v4: Add Acked-by and Tested-by in signatures.
v3: same as v1
v2: same as v1
drivers/usb/core/usb-acpi.c | 44 +++++++++++++++++++++++++++++--------
1 file changed, 35 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 8ff73c83e8e8..9043d7242d67 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -200,30 +200,56 @@ static struct acpi_device *
usb_acpi_find_companion_for_device(struct usb_device *udev)
{
struct acpi_device *adev;
+ struct usb_port *port_dev;
+ struct usb_hub *hub;
+
+ if (!udev->parent) {
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+ }
- if (!udev->parent)
+ hub = usb_hub_to_struct_hub(udev->parent);
+ if (!hub)
return NULL;
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(udev->dev.parent);
- return acpi_find_child_device(adev, 0, false);
+ /*
+ * This is an embedded USB device connected to a port and such
+ * devices share port's ACPI companion.
+ */
+ port_dev = hub->ports[udev->portnum - 1];
+ return usb_acpi_get_companion_for_port(port_dev);
}
-
static struct acpi_device *usb_acpi_find_companion(struct device *dev)
{
/*
- * In the ACPI DSDT table, only usb root hub and usb ports are
- * acpi device nodes. The hierarchy like following.
+ * The USB hierarchy like following:
+ *
* Device (EHC1)
* Device (HUBN)
* Device (PR01)
* Device (PR11)
* Device (PR12)
+ * Device (FN12)
+ * Device (FN13)
* Device (PR13)
* ...
- * So all binding process is divided into two parts. binding
- * root hub and usb ports.
+ * where HUBN is root hub, and PRNN are USB ports and devices
+ * connected to them, and FNNN are individualk functions for
+ * connected composite USB devices. PRNN and FNNN may contain
+ * _CRS and other methods describing sideband resources for
+ * the connected device.
+ *
+ * On the kernel side both root hub and embedded USB devices are
+ * represented as instances of usb_device structure, and ports
+ * are represented as usb_port structures, so the whole process
+ * is split into 2 parts: finding companions for devices and
+ * finding companions for ports.
+ *
+ * Note that we do not handle individual functions of composite
+ * devices yet, for that we would need to assign companions to
+ * devices corresponding to USB interfaces.
*/
if (is_usb_device(dev))
return usb_acpi_find_companion_for_device(to_usb_device(dev));
--
2.20.1.321.g9e740568ce-goog
Add a hook to allow the BT driver to do device or command specific
handling in case of timeouts. This is to be used by Intel driver to
reset the device after certain number of timeouts.
Signed-off-by: Rajat Jain <[email protected]>
---
v5: Drop the quirk, and rename the hook function to cmd_timeout()
v4: same as v1
v3: same as v1
v2: same as v1
include/net/bluetooth/hci_core.h | 1 +
net/bluetooth/hci_core.c | 10 ++++++++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e5ea633ea368..624d5f2b1f36 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -437,6 +437,7 @@ struct hci_dev {
int (*post_init)(struct hci_dev *hdev);
int (*set_diag)(struct hci_dev *hdev, bool enable);
int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
+ void (*cmd_timeout)(struct hci_dev *hdev, struct hci_command_hdr *cmd);
};
#define HCI_PHY_HANDLE(handle) (handle & 0xff)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7352fe85674b..c6917f57581a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2568,16 +2568,22 @@ static void hci_cmd_timeout(struct work_struct *work)
{
struct hci_dev *hdev = container_of(work, struct hci_dev,
cmd_timer.work);
+ struct hci_command_hdr *sent = NULL;
if (hdev->sent_cmd) {
- struct hci_command_hdr *sent = (void *) hdev->sent_cmd->data;
- u16 opcode = __le16_to_cpu(sent->opcode);
+ u16 opcode;
+
+ sent = (void *) hdev->sent_cmd->data;
+ opcode = __le16_to_cpu(sent->opcode);
bt_dev_err(hdev, "command 0x%4.4x tx timeout", opcode);
} else {
bt_dev_err(hdev, "command tx timeout");
}
+ if (hdev->cmd_timeout)
+ hdev->cmd_timeout(hdev, sent);
+
atomic_set(&hdev->cmd_cnt, 1);
queue_work(hdev->workqueue, &hdev->cmd_work);
}
--
2.20.1.321.g9e740568ce-goog
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 <[email protected]>
---
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;
};
+
+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);
+
+ if (data->num_cmd_timeout < 5)
+ return;
+
+ /*
+ * 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);
+ 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;
}
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);
}
--
2.20.1.321.g9e740568ce-goog
From: Dmitry Torokhov <[email protected]>
In preparation for handling embedded USB devices let's split
usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and
usb_acpi_find_companion_for_port().
Signed-off-by: Dmitry Torokhov <[email protected]>
Signed-off-by: Rajat Jain <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Tested-by: Sukumar Ghorai <[email protected]>
---
v5: same as v4
v4: Add Acked-by and Tested-by in signatures.
v3: same as v1
v2: same as v1
drivers/usb/core/usb-acpi.c | 133 +++++++++++++++++++-----------------
1 file changed, 72 insertions(+), 61 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index e221861b3187..8ff73c83e8e8 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -139,12 +139,79 @@ static struct acpi_device *usb_acpi_find_port(struct acpi_device *parent,
return acpi_find_child_device(parent, raw, false);
}
-static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+static struct acpi_device *
+usb_acpi_get_companion_for_port(struct usb_port *port_dev)
{
struct usb_device *udev;
struct acpi_device *adev;
acpi_handle *parent_handle;
+ int port1;
+
+ /* Get the struct usb_device point of port's hub */
+ udev = to_usb_device(port_dev->dev.parent->parent);
+
+ /*
+ * The root hub ports' parent is the root hub. The non-root-hub
+ * ports' parent is the parent hub port which the hub is
+ * connected to.
+ */
+ if (!udev->parent) {
+ adev = ACPI_COMPANION(&udev->dev);
+ port1 = usb_hcd_find_raw_port_number(bus_to_hcd(udev->bus),
+ port_dev->portnum);
+ } else {
+ parent_handle = usb_get_hub_port_acpi_handle(udev->parent,
+ udev->portnum);
+ if (!parent_handle)
+ return NULL;
+
+ acpi_bus_get_device(parent_handle, &adev);
+ port1 = port_dev->portnum;
+ }
+
+ return usb_acpi_find_port(adev, port1);
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_port(struct usb_port *port_dev)
+{
+ struct acpi_device *adev;
+ struct acpi_pld_info *pld;
+ acpi_handle *handle;
+ acpi_status status;
+
+ adev = usb_acpi_get_companion_for_port(port_dev);
+ if (!adev)
+ return NULL;
+
+ handle = adev->handle;
+ status = acpi_get_physical_device_location(handle, &pld);
+ if (!ACPI_FAILURE(status) && pld) {
+ port_dev->location = USB_ACPI_LOCATION_VALID
+ | pld->group_token << 8 | pld->group_position;
+ port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
+ ACPI_FREE(pld);
+ }
+ return adev;
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_device(struct usb_device *udev)
+{
+ struct acpi_device *adev;
+
+ if (!udev->parent)
+ return NULL;
+
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+}
+
+
+static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+{
/*
* In the ACPI DSDT table, only usb root hub and usb ports are
* acpi device nodes. The hierarchy like following.
@@ -158,66 +225,10 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
* So all binding process is divided into two parts. binding
* root hub and usb ports.
*/
- if (is_usb_device(dev)) {
- udev = to_usb_device(dev);
- if (udev->parent)
- return NULL;
-
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(dev->parent);
- return acpi_find_child_device(adev, 0, false);
- } else if (is_usb_port(dev)) {
- struct usb_port *port_dev = to_usb_port(dev);
- int port1 = port_dev->portnum;
- struct acpi_pld_info *pld;
- acpi_handle *handle;
- acpi_status status;
-
- /* Get the struct usb_device point of port's hub */
- udev = to_usb_device(dev->parent->parent);
-
- /*
- * The root hub ports' parent is the root hub. The non-root-hub
- * ports' parent is the parent hub port which the hub is
- * connected to.
- */
- if (!udev->parent) {
- struct usb_hcd *hcd = bus_to_hcd(udev->bus);
- int raw;
-
- raw = usb_hcd_find_raw_port_number(hcd, port1);
-
- adev = usb_acpi_find_port(ACPI_COMPANION(&udev->dev),
- raw);
-
- if (!adev)
- return NULL;
- } else {
- parent_handle =
- usb_get_hub_port_acpi_handle(udev->parent,
- udev->portnum);
- if (!parent_handle)
- return NULL;
-
- acpi_bus_get_device(parent_handle, &adev);
-
- adev = usb_acpi_find_port(adev, port1);
-
- if (!adev)
- return NULL;
- }
- handle = adev->handle;
- status = acpi_get_physical_device_location(handle, &pld);
- if (ACPI_FAILURE(status) || !pld)
- return adev;
-
- port_dev->location = USB_ACPI_LOCATION_VALID
- | pld->group_token << 8 | pld->group_position;
- port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
- ACPI_FREE(pld);
-
- return adev;
- }
+ if (is_usb_device(dev))
+ return usb_acpi_find_companion_for_device(to_usb_device(dev));
+ else if (is_usb_port(dev))
+ return usb_acpi_find_companion_for_port(to_usb_port(dev));
return NULL;
}
--
2.20.1.321.g9e740568ce-goog
Hi Rajat,
> Add a hook to allow the BT driver to do device or command specific
> handling in case of timeouts. This is to be used by Intel driver to
> reset the device after certain number of timeouts.
>
> Signed-off-by: Rajat Jain <[email protected]>
> ---
> v5: Drop the quirk, and rename the hook function to cmd_timeout()
> v4: same as v1
> v3: same as v1
> v2: same as v1
>
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_core.c | 10 ++++++++--
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index e5ea633ea368..624d5f2b1f36 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -437,6 +437,7 @@ struct hci_dev {
> int (*post_init)(struct hci_dev *hdev);
> int (*set_diag)(struct hci_dev *hdev, bool enable);
> int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> + void (*cmd_timeout)(struct hci_dev *hdev, struct hci_command_hdr *cmd);
> };
>
> #define HCI_PHY_HANDLE(handle) (handle & 0xff)
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 7352fe85674b..c6917f57581a 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2568,16 +2568,22 @@ static void hci_cmd_timeout(struct work_struct *work)
> {
> struct hci_dev *hdev = container_of(work, struct hci_dev,
> cmd_timer.work);
> + struct hci_command_hdr *sent = NULL;
>
> if (hdev->sent_cmd) {
> - struct hci_command_hdr *sent = (void *) hdev->sent_cmd->data;
> - u16 opcode = __le16_to_cpu(sent->opcode);
> + u16 opcode;
> +
> + sent = (void *) hdev->sent_cmd->data;
> + opcode = __le16_to_cpu(sent->opcode);
>
> bt_dev_err(hdev, "command 0x%4.4x tx timeout", opcode);
> } else {
> bt_dev_err(hdev, "command tx timeout");
> }
>
> + if (hdev->cmd_timeout)
> + hdev->cmd_timeout(hdev, sent);
> +
drop the sent parameter. You are not using it and if at some point in the future you might want to use it, then we add it and fix up all users.
And frankly, I would move the hdev->cmd_timeout call into the hdev->sent_cmd block since I do not even know if it makes sense to deal with the fallback case where hdev->sent_cmd is not available. Unless you have that kind of error case, but that is only possible if you have external injection of HCI commands.
Regards
Marcel
Hi Rajat,
> 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 <[email protected]>
> ---
> 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;
> +
> + /*
> + * 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?
> + 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.
>
> 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
On Thu, Jan 24, 2019 at 1:46 AM Marcel Holtmann <[email protected]> wrote:
>
> Hi Rajat,
>
> > 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 <[email protected]>
> > ---
> > 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.
Sure, will do.
>
> > };
> >
> > +
> > +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.
will do.
>
> > +
> > + if (data->num_cmd_timeout < 5)
> > + return;
>
> So I would propose to do just this:
>
> if (++data->cmd_timeout_count < 5)
> return;
will do.
>
> > +
> > + /*
> > + * 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?
Yes, we're using the &data->udev->dev device (parent of the USB
interface) to fetch the gpio, so if we use the devm_* variant, the
gpio resource will not be freed immediately if the btusb_probe fails.
I'll add a comment in the code to make this more clear.
>
> > + 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.
will do.
>
> Move the if (data->reset_gpio) check into btusb_intel_cmd_timeout() function and print a warning that no reset GPIO is present.
There'll be Intel platforms already out there which will obviously not
have the reset pin described in the ACPI as of today. So I'm not sure
if it is right for all of them to start complaining about a missing
"reset gpio" in case of timeout post this change. Thus I would prefer
to assign hdev->cmd_timeout only if ACPI does provide a gpio i.e.
newer platforms that want to support this feature. Thoughts?
Thanks,
Rajat
>
> >
> > 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
>
Hi Marcel,
On Thu, Jan 24, 2019 at 1:36 AM Marcel Holtmann <[email protected]> wrote:
>
> Hi Rajat,
>
> > Add a hook to allow the BT driver to do device or command specific
> > handling in case of timeouts. This is to be used by Intel driver to
> > reset the device after certain number of timeouts.
> >
> > Signed-off-by: Rajat Jain <[email protected]>
> > ---
> > v5: Drop the quirk, and rename the hook function to cmd_timeout()
> > v4: same as v1
> > v3: same as v1
> > v2: same as v1
> >
> > include/net/bluetooth/hci_core.h | 1 +
> > net/bluetooth/hci_core.c | 10 ++++++++--
> > 2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index e5ea633ea368..624d5f2b1f36 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -437,6 +437,7 @@ struct hci_dev {
> > int (*post_init)(struct hci_dev *hdev);
> > int (*set_diag)(struct hci_dev *hdev, bool enable);
> > int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> > + void (*cmd_timeout)(struct hci_dev *hdev, struct hci_command_hdr *cmd);
> > };
> >
> > #define HCI_PHY_HANDLE(handle) (handle & 0xff)
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 7352fe85674b..c6917f57581a 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -2568,16 +2568,22 @@ static void hci_cmd_timeout(struct work_struct *work)
> > {
> > struct hci_dev *hdev = container_of(work, struct hci_dev,
> > cmd_timer.work);
> > + struct hci_command_hdr *sent = NULL;
> >
> > if (hdev->sent_cmd) {
> > - struct hci_command_hdr *sent = (void *) hdev->sent_cmd->data;
> > - u16 opcode = __le16_to_cpu(sent->opcode);
> > + u16 opcode;
> > +
> > + sent = (void *) hdev->sent_cmd->data;
> > + opcode = __le16_to_cpu(sent->opcode);
> >
> > bt_dev_err(hdev, "command 0x%4.4x tx timeout", opcode);
> > } else {
> > bt_dev_err(hdev, "command tx timeout");
> > }
> >
> > + if (hdev->cmd_timeout)
> > + hdev->cmd_timeout(hdev, sent);
> > +
>
> drop the sent parameter. You are not using it and if at some point in the future you might want to use it, then we add it and fix up all users.
Sure, will do.
>
> And frankly, I would move the hdev->cmd_timeout call into the hdev->sent_cmd block since I do not even know if it makes sense to deal with the fallback case where hdev->sent_cmd is not available. Unless you have that kind of error case, but that is only possible if you have external injection of HCI commands.
Ummm ... my preference would be to keep it outside the block so that
the hook is always invoked in any timeout, to be consistent (whether
externally injected or not). Let me know if that is OK. I plan to send
out an iteration keeping it outside, but I'll be happy to move it in
if you feel strongly about it.
Thanks,
Rajat
>
> Regards
>
> Marcel
>
From: Dmitry Torokhov <[email protected]>
USB devices permanently connected to USB ports may be described in ACPI
tables and share ACPI devices with ports they are connected to. See [1]
for details.
This will allow us to describe sideband resources for devices, such as,
for example, hard reset line for BT USB controllers.
[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects#acpi-namespace-hierarchy-and-adr-for-embedded-usb-devices
Signed-off-by: Dmitry Torokhov <[email protected]>
Signed-off-by: Rajat Jain <[email protected]> (changed how we get the usb_port)
Acked-by: Greg Kroah-Hartman <[email protected]>
Tested-by: Sukumar Ghorai <[email protected]>
---
v6: same as v4
v5: same as v4
v4: Add Acked-by and Tested-by in signatures.
v3: same as v1
v2: same as v1
drivers/usb/core/usb-acpi.c | 44 +++++++++++++++++++++++++++++--------
1 file changed, 35 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 8ff73c83e8e8..9043d7242d67 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -200,30 +200,56 @@ static struct acpi_device *
usb_acpi_find_companion_for_device(struct usb_device *udev)
{
struct acpi_device *adev;
+ struct usb_port *port_dev;
+ struct usb_hub *hub;
+
+ if (!udev->parent) {
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+ }
- if (!udev->parent)
+ hub = usb_hub_to_struct_hub(udev->parent);
+ if (!hub)
return NULL;
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(udev->dev.parent);
- return acpi_find_child_device(adev, 0, false);
+ /*
+ * This is an embedded USB device connected to a port and such
+ * devices share port's ACPI companion.
+ */
+ port_dev = hub->ports[udev->portnum - 1];
+ return usb_acpi_get_companion_for_port(port_dev);
}
-
static struct acpi_device *usb_acpi_find_companion(struct device *dev)
{
/*
- * In the ACPI DSDT table, only usb root hub and usb ports are
- * acpi device nodes. The hierarchy like following.
+ * The USB hierarchy like following:
+ *
* Device (EHC1)
* Device (HUBN)
* Device (PR01)
* Device (PR11)
* Device (PR12)
+ * Device (FN12)
+ * Device (FN13)
* Device (PR13)
* ...
- * So all binding process is divided into two parts. binding
- * root hub and usb ports.
+ * where HUBN is root hub, and PRNN are USB ports and devices
+ * connected to them, and FNNN are individualk functions for
+ * connected composite USB devices. PRNN and FNNN may contain
+ * _CRS and other methods describing sideband resources for
+ * the connected device.
+ *
+ * On the kernel side both root hub and embedded USB devices are
+ * represented as instances of usb_device structure, and ports
+ * are represented as usb_port structures, so the whole process
+ * is split into 2 parts: finding companions for devices and
+ * finding companions for ports.
+ *
+ * Note that we do not handle individual functions of composite
+ * devices yet, for that we would need to assign companions to
+ * devices corresponding to USB interfaces.
*/
if (is_usb_device(dev))
return usb_acpi_find_companion_for_device(to_usb_device(dev));
--
2.20.1.321.g9e740568ce-goog
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 <[email protected]>
---
v6: Move the cmd_timeout() hook assignment with other hooks, move the
reset_gpio check in the timeout function.
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..5de0c2e59b97 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,41 @@ struct btusb_data {
int (*setup_on_usb)(struct hci_dev *hdev);
int oob_wake_irq; /* irq for out-of-band wake-on-bt */
+ unsigned cmd_timeout_cnt;
};
+
+static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
+{
+ struct btusb_data *data = hci_get_drvdata(hdev);
+ struct gpio_desc *reset_gpio = data->reset_gpio;
+
+ if (++data->cmd_timeout_cnt < 5)
+ return;
+
+ if (!reset_gpio) {
+ bt_dev_err(hdev, "No way to reset. Ignoring and continuing");
+ return;
+ }
+
+ /*
+ * 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 +2952,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 +3066,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);
+ 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;
@@ -3082,6 +3129,7 @@ static int btusb_probe(struct usb_interface *intf,
hdev->shutdown = btusb_shutdown_intel;
hdev->set_diag = btintel_set_diag_mfg;
hdev->set_bdaddr = btintel_set_bdaddr;
+ hdev->cmd_timeout = btusb_intel_cmd_timeout;
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);
@@ -3094,6 +3142,7 @@ static int btusb_probe(struct usb_interface *intf,
hdev->hw_error = btintel_hw_error;
hdev->set_diag = btintel_set_diag;
hdev->set_bdaddr = btintel_set_bdaddr;
+ hdev->cmd_timeout = btusb_intel_cmd_timeout;
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);
@@ -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);
}
--
2.20.1.321.g9e740568ce-goog
From: Dmitry Torokhov <[email protected]>
In preparation for handling embedded USB devices let's split
usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and
usb_acpi_find_companion_for_port().
Signed-off-by: Dmitry Torokhov <[email protected]>
Signed-off-by: Rajat Jain <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Tested-by: Sukumar Ghorai <[email protected]>
---
v6: same as v4
v5: same as v4
v4: Add Acked-by and Tested-by in signatures.
v3: same as v1
v2: same as v1
drivers/usb/core/usb-acpi.c | 133 +++++++++++++++++++-----------------
1 file changed, 72 insertions(+), 61 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index e221861b3187..8ff73c83e8e8 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -139,12 +139,79 @@ static struct acpi_device *usb_acpi_find_port(struct acpi_device *parent,
return acpi_find_child_device(parent, raw, false);
}
-static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+static struct acpi_device *
+usb_acpi_get_companion_for_port(struct usb_port *port_dev)
{
struct usb_device *udev;
struct acpi_device *adev;
acpi_handle *parent_handle;
+ int port1;
+
+ /* Get the struct usb_device point of port's hub */
+ udev = to_usb_device(port_dev->dev.parent->parent);
+
+ /*
+ * The root hub ports' parent is the root hub. The non-root-hub
+ * ports' parent is the parent hub port which the hub is
+ * connected to.
+ */
+ if (!udev->parent) {
+ adev = ACPI_COMPANION(&udev->dev);
+ port1 = usb_hcd_find_raw_port_number(bus_to_hcd(udev->bus),
+ port_dev->portnum);
+ } else {
+ parent_handle = usb_get_hub_port_acpi_handle(udev->parent,
+ udev->portnum);
+ if (!parent_handle)
+ return NULL;
+
+ acpi_bus_get_device(parent_handle, &adev);
+ port1 = port_dev->portnum;
+ }
+
+ return usb_acpi_find_port(adev, port1);
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_port(struct usb_port *port_dev)
+{
+ struct acpi_device *adev;
+ struct acpi_pld_info *pld;
+ acpi_handle *handle;
+ acpi_status status;
+
+ adev = usb_acpi_get_companion_for_port(port_dev);
+ if (!adev)
+ return NULL;
+
+ handle = adev->handle;
+ status = acpi_get_physical_device_location(handle, &pld);
+ if (!ACPI_FAILURE(status) && pld) {
+ port_dev->location = USB_ACPI_LOCATION_VALID
+ | pld->group_token << 8 | pld->group_position;
+ port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
+ ACPI_FREE(pld);
+ }
+ return adev;
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_device(struct usb_device *udev)
+{
+ struct acpi_device *adev;
+
+ if (!udev->parent)
+ return NULL;
+
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+}
+
+
+static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+{
/*
* In the ACPI DSDT table, only usb root hub and usb ports are
* acpi device nodes. The hierarchy like following.
@@ -158,66 +225,10 @@ static struct acpi_device *usb_acpi_find_companion(struct device *dev)
* So all binding process is divided into two parts. binding
* root hub and usb ports.
*/
- if (is_usb_device(dev)) {
- udev = to_usb_device(dev);
- if (udev->parent)
- return NULL;
-
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(dev->parent);
- return acpi_find_child_device(adev, 0, false);
- } else if (is_usb_port(dev)) {
- struct usb_port *port_dev = to_usb_port(dev);
- int port1 = port_dev->portnum;
- struct acpi_pld_info *pld;
- acpi_handle *handle;
- acpi_status status;
-
- /* Get the struct usb_device point of port's hub */
- udev = to_usb_device(dev->parent->parent);
-
- /*
- * The root hub ports' parent is the root hub. The non-root-hub
- * ports' parent is the parent hub port which the hub is
- * connected to.
- */
- if (!udev->parent) {
- struct usb_hcd *hcd = bus_to_hcd(udev->bus);
- int raw;
-
- raw = usb_hcd_find_raw_port_number(hcd, port1);
-
- adev = usb_acpi_find_port(ACPI_COMPANION(&udev->dev),
- raw);
-
- if (!adev)
- return NULL;
- } else {
- parent_handle =
- usb_get_hub_port_acpi_handle(udev->parent,
- udev->portnum);
- if (!parent_handle)
- return NULL;
-
- acpi_bus_get_device(parent_handle, &adev);
-
- adev = usb_acpi_find_port(adev, port1);
-
- if (!adev)
- return NULL;
- }
- handle = adev->handle;
- status = acpi_get_physical_device_location(handle, &pld);
- if (ACPI_FAILURE(status) || !pld)
- return adev;
-
- port_dev->location = USB_ACPI_LOCATION_VALID
- | pld->group_token << 8 | pld->group_position;
- port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
- ACPI_FREE(pld);
-
- return adev;
- }
+ if (is_usb_device(dev))
+ return usb_acpi_find_companion_for_device(to_usb_device(dev));
+ else if (is_usb_port(dev))
+ return usb_acpi_find_companion_for_port(to_usb_port(dev));
return NULL;
}
--
2.20.1.321.g9e740568ce-goog
Add a hook to allow the BT driver to do device or command specific
handling in case of timeouts. This is to be used by Intel driver to
reset the device after certain number of timeouts.
Signed-off-by: Rajat Jain <[email protected]>
---
v6: Dropped the "sent command" parameter from cmd_timeout()
v5: Drop the quirk, and rename the hook function to cmd_timeout()
v4: same as v1
v3: same as v1
v2: same as v1
include/net/bluetooth/hci_core.h | 1 +
net/bluetooth/hci_core.c | 3 +++
2 files changed, 4 insertions(+)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e5ea633ea368..094e61e07030 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -437,6 +437,7 @@ struct hci_dev {
int (*post_init)(struct hci_dev *hdev);
int (*set_diag)(struct hci_dev *hdev, bool enable);
int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
+ void (*cmd_timeout)(struct hci_dev *hdev);
};
#define HCI_PHY_HANDLE(handle) (handle & 0xff)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7352fe85674b..75793265ba9e 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2578,6 +2578,9 @@ static void hci_cmd_timeout(struct work_struct *work)
bt_dev_err(hdev, "command tx timeout");
}
+ if (hdev->cmd_timeout)
+ hdev->cmd_timeout(hdev);
+
atomic_set(&hdev->cmd_cnt, 1);
queue_work(hdev->workqueue, &hdev->cmd_work);
}
--
2.20.1.321.g9e740568ce-goog
Hi Rajat,
> In preparation for handling embedded USB devices let's split
> usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and
> usb_acpi_find_companion_for_port().
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> Signed-off-by: Rajat Jain <[email protected]>
> Acked-by: Greg Kroah-Hartman <[email protected]>
> Tested-by: Sukumar Ghorai <[email protected]>
> ---
> v6: same as v4
> v5: same as v4
> v4: Add Acked-by and Tested-by in signatures.
> v3: same as v1
> v2: same as v1
>
> drivers/usb/core/usb-acpi.c | 133 +++++++++++++++++++-----------------
> 1 file changed, 72 insertions(+), 61 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
Hi Rajat,
> USB devices permanently connected to USB ports may be described in ACPI
> tables and share ACPI devices with ports they are connected to. See [1]
> for details.
>
> This will allow us to describe sideband resources for devices, such as,
> for example, hard reset line for BT USB controllers.
>
> [1] https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects#acpi-namespace-hierarchy-and-adr-for-embedded-usb-devices
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> Signed-off-by: Rajat Jain <[email protected]> (changed how we get the usb_port)
> Acked-by: Greg Kroah-Hartman <[email protected]>
> Tested-by: Sukumar Ghorai <[email protected]>
> ---
> v6: same as v4
> v5: same as v4
> v4: Add Acked-by and Tested-by in signatures.
> v3: same as v1
> v2: same as v1
>
> drivers/usb/core/usb-acpi.c | 44 +++++++++++++++++++++++++++++--------
> 1 file changed, 35 insertions(+), 9 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
Hi Rajat,
> 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 <[email protected]>
> ---
> v6: Move the cmd_timeout() hook assignment with other hooks, move the
> reset_gpio check in the timeout function.
> 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(+)
patch has been applied to bluetooth-next tree.
Regards
Marcel
Hi Rajat,
> Add a hook to allow the BT driver to do device or command specific
> handling in case of timeouts. This is to be used by Intel driver to
> reset the device after certain number of timeouts.
>
> Signed-off-by: Rajat Jain <[email protected]>
> ---
> v6: Dropped the "sent command" parameter from cmd_timeout()
> v5: Drop the quirk, and rename the hook function to cmd_timeout()
> v4: same as v1
> v3: same as v1
> v2: same as v1
>
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_core.c | 3 +++
> 2 files changed, 4 insertions(+)
patch has been applied to bluetooth-next tree.
Regards
Marcel