linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] Reset Intel BT controller if it gets stuck
@ 2018-11-17  1:07 Rajat Jain
  2018-11-17  1:07 ` [PATCH 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
                   ` (9 more replies)
  0 siblings, 10 replies; 56+ messages in thread
From: Rajat Jain @ 2018-11-17  1:07 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

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


^ permalink raw reply	[flat|nested] 56+ messages in thread

* [PATCH 1/5] usb: split code locating ACPI companion into port and device
  2018-11-17  1:07 [PATCH 0/5] Reset Intel BT controller if it gets stuck Rajat Jain
@ 2018-11-17  1:07 ` Rajat Jain
  2018-11-17  1:07 ` [PATCH 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2018-11-17  1:07 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

From: Dmitry Torokhov <dtor@chromium.org>

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 <dtor@chromium.org>
Signed-off-by: Rajat Jain <rajatja@google.com>
---
 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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH 2/5] usb: assign ACPI companions for embedded USB devices
  2018-11-17  1:07 [PATCH 0/5] Reset Intel BT controller if it gets stuck Rajat Jain
  2018-11-17  1:07 ` [PATCH 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
@ 2018-11-17  1:07 ` Rajat Jain
  2018-11-17  1:07 ` [PATCH 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts Rajat Jain
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2018-11-17  1:07 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

From: Dmitry Torokhov <dtor@chromium.org>

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 <dtor@chromium.org>
Signed-off-by: Rajat Jain <rajatja@google.com> (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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts
  2018-11-17  1:07 [PATCH 0/5] Reset Intel BT controller if it gets stuck Rajat Jain
  2018-11-17  1:07 ` [PATCH 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
  2018-11-17  1:07 ` [PATCH 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
@ 2018-11-17  1:07 ` Rajat Jain
  2018-11-17  1:07 ` [PATCH 4/5] Bluetooth: btusb: Collect the common Intel assignments together Rajat Jain
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2018-11-17  1:07 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
---
 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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH 4/5] Bluetooth: btusb: Collect the common Intel assignments together
  2018-11-17  1:07 [PATCH 0/5] Reset Intel BT controller if it gets stuck Rajat Jain
                   ` (2 preceding siblings ...)
  2018-11-17  1:07 ` [PATCH 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts Rajat Jain
@ 2018-11-17  1:07 ` Rajat Jain
  2018-11-17  1:07 ` [PATCH 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip Rajat Jain
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2018-11-17  1:07 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
---
 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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip
  2018-11-17  1:07 [PATCH 0/5] Reset Intel BT controller if it gets stuck Rajat Jain
                   ` (3 preceding siblings ...)
  2018-11-17  1:07 ` [PATCH 4/5] Bluetooth: btusb: Collect the common Intel assignments together Rajat Jain
@ 2018-11-17  1:07 ` Rajat Jain
  2018-11-19 23:04 ` [PATCH v2 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2018-11-17  1:07 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

If the platform provides it, use the reset gpio to reset the BT
chip (requested by the HCI core if needed). This has been found helpful
on some of Intel bluetooth controllers where the firmware gets stuck and
the only way out is a hard reset pin provided by the platform.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
 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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v2 1/5] usb: split code locating ACPI companion into port and device
  2018-11-17  1:07 [PATCH 0/5] Reset Intel BT controller if it gets stuck Rajat Jain
                   ` (4 preceding siblings ...)
  2018-11-17  1:07 ` [PATCH 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip Rajat Jain
@ 2018-11-19 23:04 ` Rajat Jain
  2018-11-19 23:04   ` [PATCH v2 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
                     ` (3 more replies)
  2018-11-21 23:50 ` [PATCH v3 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
                   ` (3 subsequent siblings)
  9 siblings, 4 replies; 56+ messages in thread
From: Rajat Jain @ 2018-11-19 23:04 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

From: Dmitry Torokhov <dtor@chromium.org>

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 <dtor@chromium.org>
Signed-off-by: Rajat Jain <rajatja@google.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v2 2/5] usb: assign ACPI companions for embedded USB devices
  2018-11-19 23:04 ` [PATCH v2 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
@ 2018-11-19 23:04   ` Rajat Jain
  2018-11-19 23:04   ` [PATCH v2 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts Rajat Jain
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2018-11-19 23:04 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

From: Dmitry Torokhov <dtor@chromium.org>

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 <dtor@chromium.org>
Signed-off-by: Rajat Jain <rajatja@google.com> (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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v2 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts
  2018-11-19 23:04 ` [PATCH v2 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
  2018-11-19 23:04   ` [PATCH v2 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
@ 2018-11-19 23:04   ` Rajat Jain
  2018-11-19 23:04   ` [PATCH v2 4/5] Bluetooth: btusb: Collect the common Intel assignments together Rajat Jain
  2018-11-19 23:04   ` [PATCH v2 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip Rajat Jain
  3 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2018-11-19 23:04 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v2 4/5] Bluetooth: btusb: Collect the common Intel assignments together
  2018-11-19 23:04 ` [PATCH v2 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
  2018-11-19 23:04   ` [PATCH v2 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
  2018-11-19 23:04   ` [PATCH v2 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts Rajat Jain
@ 2018-11-19 23:04   ` Rajat Jain
  2018-11-19 23:04   ` [PATCH v2 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip Rajat Jain
  3 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2018-11-19 23:04 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v2 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip
  2018-11-19 23:04 ` [PATCH v2 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
                     ` (2 preceding siblings ...)
  2018-11-19 23:04   ` [PATCH v2 4/5] Bluetooth: btusb: Collect the common Intel assignments together Rajat Jain
@ 2018-11-19 23:04   ` Rajat Jain
  3 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2018-11-19 23:04 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

If the platform provides it, use the reset gpio to reset the BT
chip (requested by the HCI core if needed). This has been found helpful
on some of Intel bluetooth controllers where the firmware gets stuck and
the only way out is a hard reset pin provided by the platform.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v3 1/5] usb: split code locating ACPI companion into port and device
  2018-11-17  1:07 [PATCH 0/5] Reset Intel BT controller if it gets stuck Rajat Jain
                   ` (5 preceding siblings ...)
  2018-11-19 23:04 ` [PATCH v2 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
@ 2018-11-21 23:50 ` Rajat Jain
  2018-11-21 23:50   ` [PATCH v3 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
                     ` (4 more replies)
  2019-01-18 22:34 ` [PATCH v4 " Rajat Jain
                   ` (2 subsequent siblings)
  9 siblings, 5 replies; 56+ messages in thread
From: Rajat Jain @ 2018-11-21 23:50 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

From: Dmitry Torokhov <dtor@chromium.org>

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 <dtor@chromium.org>
Signed-off-by: Rajat Jain <rajatja@google.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v3 2/5] usb: assign ACPI companions for embedded USB devices
  2018-11-21 23:50 ` [PATCH v3 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
@ 2018-11-21 23:50   ` Rajat Jain
  2018-12-05  9:32     ` Greg Kroah-Hartman
  2018-11-21 23:50   ` [PATCH v3 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts Rajat Jain
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 56+ messages in thread
From: Rajat Jain @ 2018-11-21 23:50 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

From: Dmitry Torokhov <dtor@chromium.org>

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 <dtor@chromium.org>
Signed-off-by: Rajat Jain <rajatja@google.com> (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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v3 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts
  2018-11-21 23:50 ` [PATCH v3 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
  2018-11-21 23:50   ` [PATCH v3 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
@ 2018-11-21 23:50   ` Rajat Jain
  2018-11-21 23:50   ` [PATCH v3 4/5] Bluetooth: btusb: Collect the common Intel assignments together Rajat Jain
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2018-11-21 23:50 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v3 4/5] Bluetooth: btusb: Collect the common Intel assignments together
  2018-11-21 23:50 ` [PATCH v3 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
  2018-11-21 23:50   ` [PATCH v3 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
  2018-11-21 23:50   ` [PATCH v3 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts Rajat Jain
@ 2018-11-21 23:50   ` Rajat Jain
  2018-11-21 23:50   ` [PATCH v3 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip Rajat Jain
  2018-12-05  9:32   ` [PATCH v3 1/5] usb: split code locating ACPI companion into port and device Greg Kroah-Hartman
  4 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2018-11-21 23:50 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v3 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip
  2018-11-21 23:50 ` [PATCH v3 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
                     ` (2 preceding siblings ...)
  2018-11-21 23:50   ` [PATCH v3 4/5] Bluetooth: btusb: Collect the common Intel assignments together Rajat Jain
@ 2018-11-21 23:50   ` Rajat Jain
  2018-12-20  8:46     ` Rajat Jain
  2019-01-18 11:04     ` Marcel Holtmann
  2018-12-05  9:32   ` [PATCH v3 1/5] usb: split code locating ACPI companion into port and device Greg Kroah-Hartman
  4 siblings, 2 replies; 56+ messages in thread
From: Rajat Jain @ 2018-11-21 23:50 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

If the platform provides it, use the reset gpio to reset the BT
chip (requested by the HCI core if needed). This has been found helpful
on some of Intel bluetooth controllers where the firmware gets stuck and
the only way out is a hard reset pin provided by the platform.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
v3: Better error handling for gpiod_get_optional()
v2: Handle the EPROBE_DEFER case.

 drivers/bluetooth/btusb.c | 40 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index e8e148480c91..e7631f770fae 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -29,6 +29,7 @@
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
 #include <linux/suspend.h>
+#include <linux/gpio/consumer.h>
 #include <asm/unaligned.h>
 
 #include <net/bluetooth/bluetooth.h>
@@ -475,6 +476,8 @@ struct btusb_data {
 	struct usb_endpoint_descriptor *diag_tx_ep;
 	struct usb_endpoint_descriptor *diag_rx_ep;
 
+	struct gpio_desc *reset_gpio;
+
 	__u8 cmdreq_type;
 	__u8 cmdreq;
 
@@ -490,6 +493,26 @@ struct btusb_data {
 	int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
 };
 
+
+static void btusb_hw_reset(struct hci_dev *hdev)
+{
+	struct btusb_data *data = hci_get_drvdata(hdev);
+	struct gpio_desc *reset_gpio = data->reset_gpio;
+
+	/*
+	 * Toggle the hard reset line if the platform provides one. The reset
+	 * is going to yank the device off the USB and then replug. So doing
+	 * once is enough. The cleanup is handled correctly on the way out
+	 * (standard USB disconnect), and the new device is detected cleanly
+	 * and bound to the driver again like it should be.
+	 */
+	bt_dev_dbg(hdev, "%s: Initiating HW reset via gpio", __func__);
+	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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* Re: [PATCH v3 1/5] usb: split code locating ACPI companion into port and device
  2018-11-21 23:50 ` [PATCH v3 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
                     ` (3 preceding siblings ...)
  2018-11-21 23:50   ` [PATCH v3 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip Rajat Jain
@ 2018-12-05  9:32   ` Greg Kroah-Hartman
  2018-12-05 17:41     ` Ghorai, Sukumar
  4 siblings, 1 reply; 56+ messages in thread
From: Greg Kroah-Hartman @ 2018-12-05  9:32 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Marcel Holtmann, Johan Hedberg, David S. Miller, Dmitry Torokhov,
	Alex Hung, linux-bluetooth, linux-kernel, linux-usb, netdev,
	rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan,
	sukumar.ghorai

On Wed, Nov 21, 2018 at 03:50:16PM -0800, Rajat Jain wrote:
> From: Dmitry Torokhov <dtor@chromium.org>
> 
> 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 <dtor@chromium.org>
> Signed-off-by: Rajat Jain <rajatja@google.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v3 2/5] usb: assign ACPI companions for embedded USB devices
  2018-11-21 23:50   ` [PATCH v3 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
@ 2018-12-05  9:32     ` Greg Kroah-Hartman
  2018-12-05 17:19       ` Ghorai, Sukumar
  0 siblings, 1 reply; 56+ messages in thread
From: Greg Kroah-Hartman @ 2018-12-05  9:32 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Marcel Holtmann, Johan Hedberg, David S. Miller, Dmitry Torokhov,
	Alex Hung, linux-bluetooth, linux-kernel, linux-usb, netdev,
	rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan,
	sukumar.ghorai

On Wed, Nov 21, 2018 at 03:50:17PM -0800, Rajat Jain wrote:
> From: Dmitry Torokhov <dtor@chromium.org>
> 
> 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 <dtor@chromium.org>
> Signed-off-by: Rajat Jain <rajatja@google.com> (changed how we get the usb_port)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* RE: [PATCH v3 2/5] usb: assign ACPI companions for embedded USB devices
  2018-12-05  9:32     ` Greg Kroah-Hartman
@ 2018-12-05 17:19       ` Ghorai, Sukumar
  0 siblings, 0 replies; 56+ messages in thread
From: Ghorai, Sukumar @ 2018-12-05 17:19 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rajat Jain
  Cc: Marcel Holtmann, Johan Hedberg, David S. Miller, Dmitry Torokhov,
	Alex Hung, linux-bluetooth, linux-kernel, linux-usb, netdev,
	rajatxjain, dtor, Hegde, Raghuram, Tumkur Narayan, Chethan

>On Wed, Nov 21, 2018 at 03:50:17PM -0800, Rajat Jain wrote:
>> From: Dmitry Torokhov <dtor@chromium.org>
>>
>> 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 <dtor@chromium.org>
>> Signed-off-by: Rajat Jain <rajatja@google.com> (changed how we get the
>> usb_port)
>
>Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* RE: [PATCH v3 1/5] usb: split code locating ACPI companion into port and device
  2018-12-05  9:32   ` [PATCH v3 1/5] usb: split code locating ACPI companion into port and device Greg Kroah-Hartman
@ 2018-12-05 17:41     ` Ghorai, Sukumar
  0 siblings, 0 replies; 56+ messages in thread
From: Ghorai, Sukumar @ 2018-12-05 17:41 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rajat Jain
  Cc: Marcel Holtmann, Johan Hedberg, David S. Miller, Dmitry Torokhov,
	Alex Hung, linux-bluetooth, linux-kernel, linux-usb, netdev,
	rajatxjain, dtor, Hegde, Raghuram, Tumkur Narayan, Chethan

>On Wed, Nov 21, 2018 at 03:50:16PM -0800, Rajat Jain wrote:
>> From: Dmitry Torokhov <dtor@chromium.org>
>>
>> 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 <dtor@chromium.org>
>> Signed-off-by: Rajat Jain <rajatja@google.com>
>
>Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v3 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip
  2018-11-21 23:50   ` [PATCH v3 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip Rajat Jain
@ 2018-12-20  8:46     ` Rajat Jain
  2019-01-18 11:04     ` Marcel Holtmann
  1 sibling, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2018-12-20  8:46 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Alex Hung, linux-bluetooth,
	Linux Kernel Mailing List, linux-usb, netdev, Dmitry Torokhov,
	raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

On Wed, Nov 21, 2018 at 3:50 PM Rajat Jain <rajatja@google.com> wrote:
>
> If the platform provides it, use the reset gpio to reset the BT
> chip (requested by the HCI core if needed). This has been found helpful
> on some of Intel bluetooth controllers where the firmware gets stuck and
> the only way out is a hard reset pin provided by the platform.

Hi Folks,

I was wondering if you got a change to look at this patchset, and if
there is any feedback.

Thanks,

Rajat


>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v3: Better error handling for gpiod_get_optional()
> v2: Handle the EPROBE_DEFER case.
>
>  drivers/bluetooth/btusb.c | 40 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index e8e148480c91..e7631f770fae 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -29,6 +29,7 @@
>  #include <linux/of_device.h>
>  #include <linux/of_irq.h>
>  #include <linux/suspend.h>
> +#include <linux/gpio/consumer.h>
>  #include <asm/unaligned.h>
>
>  #include <net/bluetooth/bluetooth.h>
> @@ -475,6 +476,8 @@ struct btusb_data {
>         struct usb_endpoint_descriptor *diag_tx_ep;
>         struct usb_endpoint_descriptor *diag_rx_ep;
>
> +       struct gpio_desc *reset_gpio;
> +
>         __u8 cmdreq_type;
>         __u8 cmdreq;
>
> @@ -490,6 +493,26 @@ struct btusb_data {
>         int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
>  };
>
> +
> +static void btusb_hw_reset(struct hci_dev *hdev)
> +{
> +       struct btusb_data *data = hci_get_drvdata(hdev);
> +       struct gpio_desc *reset_gpio = data->reset_gpio;
> +
> +       /*
> +        * Toggle the hard reset line if the platform provides one. The reset
> +        * is going to yank the device off the USB and then replug. So doing
> +        * once is enough. The cleanup is handled correctly on the way out
> +        * (standard USB disconnect), and the new device is detected cleanly
> +        * and bound to the driver again like it should be.
> +        */
> +       bt_dev_dbg(hdev, "%s: Initiating HW reset via gpio", __func__);
> +       clear_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks);
> +       gpiod_set_value(reset_gpio, 1);
> +       mdelay(100);
> +       gpiod_set_value(reset_gpio, 0);
> +}
> +
>  static inline void btusb_free_frags(struct btusb_data *data)
>  {
>         unsigned long flags;
> @@ -2917,6 +2940,7 @@ static int btusb_probe(struct usb_interface *intf,
>                        const struct usb_device_id *id)
>  {
>         struct usb_endpoint_descriptor *ep_desc;
> +       struct gpio_desc *reset_gpio;
>         struct btusb_data *data;
>         struct hci_dev *hdev;
>         unsigned ifnum_base;
> @@ -3030,6 +3054,16 @@ static int btusb_probe(struct usb_interface *intf,
>
>         SET_HCIDEV_DEV(hdev, &intf->dev);
>
> +       reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
> +                                       GPIOD_OUT_LOW);
> +       if (IS_ERR(reset_gpio)) {
> +               err = PTR_ERR(reset_gpio);
> +               goto out_free_dev;
> +       } else if (reset_gpio) {
> +               data->reset_gpio = reset_gpio;
> +               hdev->hw_reset = btusb_hw_reset;
> +       }
> +
>         hdev->open   = btusb_open;
>         hdev->close  = btusb_close;
>         hdev->flush  = btusb_flush;
> @@ -3085,6 +3119,7 @@ static int btusb_probe(struct usb_interface *intf,
>                 set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
>                 set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>                 set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
> +               set_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks);
>
>                 if (id->driver_info & BTUSB_INTEL) {
>                         hdev->setup = btusb_setup_intel;
> @@ -3225,6 +3260,8 @@ static int btusb_probe(struct usb_interface *intf,
>         return 0;
>
>  out_free_dev:
> +       if (data->reset_gpio)
> +               gpiod_put(data->reset_gpio);
>         hci_free_dev(hdev);
>         return err;
>  }
> @@ -3268,6 +3305,9 @@ static void btusb_disconnect(struct usb_interface *intf)
>         if (data->oob_wake_irq)
>                 device_init_wakeup(&data->udev->dev, false);
>
> +       if (data->reset_gpio)
> +               gpiod_put(data->reset_gpio);
> +
>         hci_free_dev(hdev);
>  }
>
> --
> 2.19.1.1215.g8438c0b245-goog
>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v3 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip
  2018-11-21 23:50   ` [PATCH v3 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip Rajat Jain
  2018-12-20  8:46     ` Rajat Jain
@ 2019-01-18 11:04     ` Marcel Holtmann
  2019-01-18 20:51       ` Rajat Jain
  1 sibling, 1 reply; 56+ messages in thread
From: Marcel Holtmann @ 2019-01-18 11:04 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, linux-bluetooth, linux-kernel,
	linux-usb, netdev, rajatxjain, dtor, raghuram.hegde,
	chethan.tumkur.narayan, sukumar.ghorai

Hi Rajat,

> If the platform provides it, use the reset gpio to reset the BT
> chip (requested by the HCI core if needed). This has been found helpful
> on some of Intel bluetooth controllers where the firmware gets stuck and
> the only way out is a hard reset pin provided by the platform.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v3: Better error handling for gpiod_get_optional()
> v2: Handle the EPROBE_DEFER case.
> 
> drivers/bluetooth/btusb.c | 40 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index e8e148480c91..e7631f770fae 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -29,6 +29,7 @@
> #include <linux/of_device.h>
> #include <linux/of_irq.h>
> #include <linux/suspend.h>
> +#include <linux/gpio/consumer.h>
> #include <asm/unaligned.h>
> 
> #include <net/bluetooth/bluetooth.h>
> @@ -475,6 +476,8 @@ struct btusb_data {
> 	struct usb_endpoint_descriptor *diag_tx_ep;
> 	struct usb_endpoint_descriptor *diag_rx_ep;
> 
> +	struct gpio_desc *reset_gpio;
> +
> 	__u8 cmdreq_type;
> 	__u8 cmdreq;
> 
> @@ -490,6 +493,26 @@ struct btusb_data {
> 	int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
> };
> 
> +
> +static void btusb_hw_reset(struct hci_dev *hdev)
> +{
> +	struct btusb_data *data = hci_get_drvdata(hdev);
> +	struct gpio_desc *reset_gpio = data->reset_gpio;
> +
> +	/*
> +	 * Toggle the hard reset line if the platform provides one. The reset
> +	 * is going to yank the device off the USB and then replug. So doing
> +	 * once is enough. The cleanup is handled correctly on the way out
> +	 * (standard USB disconnect), and the new device is detected cleanly
> +	 * and bound to the driver again like it should be.
> +	 */
> +	bt_dev_dbg(hdev, "%s: Initiating HW reset via gpio", __func__);

No __func__ here. That bt_dev_dbg does all of that via dynamic debug already.

> +	clear_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks);
> +	gpiod_set_value(reset_gpio, 1);
> +	mdelay(100);
> +	gpiod_set_value(reset_gpio, 0);
> +}
> +
> static inline void btusb_free_frags(struct btusb_data *data)
> {
> 	unsigned long flags;
> @@ -2917,6 +2940,7 @@ static int btusb_probe(struct usb_interface *intf,
> 		       const struct usb_device_id *id)
> {
> 	struct usb_endpoint_descriptor *ep_desc;
> +	struct gpio_desc *reset_gpio;
> 	struct btusb_data *data;
> 	struct hci_dev *hdev;
> 	unsigned ifnum_base;
> @@ -3030,6 +3054,16 @@ static int btusb_probe(struct usb_interface *intf,
> 
> 	SET_HCIDEV_DEV(hdev, &intf->dev);
> 
> +	reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
> +					GPIOD_OUT_LOW);
> +	if (IS_ERR(reset_gpio)) {
> +		err = PTR_ERR(reset_gpio);
> +		goto out_free_dev;
> +	} else if (reset_gpio) {
> +		data->reset_gpio = reset_gpio;
> +		hdev->hw_reset = btusb_hw_reset;
> +	}
> +

How do we ensure that this is the right “reset” line. And it also needs to be bound to some hardware unless we can guarantee that this is always the same.

> 	hdev->open   = btusb_open;
> 	hdev->close  = btusb_close;
> 	hdev->flush  = btusb_flush;
> @@ -3085,6 +3119,7 @@ static int btusb_probe(struct usb_interface *intf,
> 		set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
> 		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
> 		set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
> +		set_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks);

You are not messing with the quirks here please. Clearing quirks is crazy. Use the data->flags since this should be all btusb.c specific.

> 
> 		if (id->driver_info & BTUSB_INTEL) {
> 			hdev->setup = btusb_setup_intel;
> @@ -3225,6 +3260,8 @@ static int btusb_probe(struct usb_interface *intf,
> 	return 0;
> 
> out_free_dev:
> +	if (data->reset_gpio)
> +		gpiod_put(data->reset_gpio);
> 	hci_free_dev(hdev);
> 	return err;
> }
> @@ -3268,6 +3305,9 @@ static void btusb_disconnect(struct usb_interface *intf)
> 	if (data->oob_wake_irq)
> 		device_init_wakeup(&data->udev->dev, false);
> 
> +	if (data->reset_gpio)
> +		gpiod_put(data->reset_gpio);
> +
> 	hci_free_dev(hdev);
> }

Regards

Marcel


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v3 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip
  2019-01-18 11:04     ` Marcel Holtmann
@ 2019-01-18 20:51       ` Rajat Jain
  0 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2019-01-18 20:51 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, linux-bluetooth,
	Linux Kernel Mailing List, linux-usb, netdev, Rajat Jain,
	Dmitry Torokhov, raghuram.hegde, chethan.tumkur.narayan, Ghorai,
	Sukumar

Hi Marcel,

Thanks for your review.

On Fri, Jan 18, 2019 at 3:04 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Rajat,
>
> > If the platform provides it, use the reset gpio to reset the BT
> > chip (requested by the HCI core if needed). This has been found helpful
> > on some of Intel bluetooth controllers where the firmware gets stuck and
> > the only way out is a hard reset pin provided by the platform.
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> > v3: Better error handling for gpiod_get_optional()
> > v2: Handle the EPROBE_DEFER case.
> >
> > drivers/bluetooth/btusb.c | 40 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 40 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index e8e148480c91..e7631f770fae 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -29,6 +29,7 @@
> > #include <linux/of_device.h>
> > #include <linux/of_irq.h>
> > #include <linux/suspend.h>
> > +#include <linux/gpio/consumer.h>
> > #include <asm/unaligned.h>
> >
> > #include <net/bluetooth/bluetooth.h>
> > @@ -475,6 +476,8 @@ struct btusb_data {
> >       struct usb_endpoint_descriptor *diag_tx_ep;
> >       struct usb_endpoint_descriptor *diag_rx_ep;
> >
> > +     struct gpio_desc *reset_gpio;
> > +
> >       __u8 cmdreq_type;
> >       __u8 cmdreq;
> >
> > @@ -490,6 +493,26 @@ struct btusb_data {
> >       int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
> > };
> >
> > +
> > +static void btusb_hw_reset(struct hci_dev *hdev)
> > +{
> > +     struct btusb_data *data = hci_get_drvdata(hdev);
> > +     struct gpio_desc *reset_gpio = data->reset_gpio;
> > +
> > +     /*
> > +      * Toggle the hard reset line if the platform provides one. The reset
> > +      * is going to yank the device off the USB and then replug. So doing
> > +      * once is enough. The cleanup is handled correctly on the way out
> > +      * (standard USB disconnect), and the new device is detected cleanly
> > +      * and bound to the driver again like it should be.
> > +      */
> > +     bt_dev_dbg(hdev, "%s: Initiating HW reset via gpio", __func__);
>
> No __func__ here. That bt_dev_dbg does all of that via dynamic debug already.

Will fix.

>
> > +     clear_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks);
> > +     gpiod_set_value(reset_gpio, 1);
> > +     mdelay(100);
> > +     gpiod_set_value(reset_gpio, 0);
> > +}
> > +
> > static inline void btusb_free_frags(struct btusb_data *data)
> > {
> >       unsigned long flags;
> > @@ -2917,6 +2940,7 @@ static int btusb_probe(struct usb_interface *intf,
> >                      const struct usb_device_id *id)
> > {
> >       struct usb_endpoint_descriptor *ep_desc;
> > +     struct gpio_desc *reset_gpio;
> >       struct btusb_data *data;
> >       struct hci_dev *hdev;
> >       unsigned ifnum_base;
> > @@ -3030,6 +3054,16 @@ static int btusb_probe(struct usb_interface *intf,
> >
> >       SET_HCIDEV_DEV(hdev, &intf->dev);
> >
> > +     reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
> > +                                     GPIOD_OUT_LOW);
> > +     if (IS_ERR(reset_gpio)) {
> > +             err = PTR_ERR(reset_gpio);
> > +             goto out_free_dev;
> > +     } else if (reset_gpio) {
> > +             data->reset_gpio = reset_gpio;
> > +             hdev->hw_reset = btusb_hw_reset;
> > +     }
> > +
>
> How do we ensure that this is the right “reset” line. And it also needs to be bound to some hardware unless
> we can guarantee that this is always the same.

The BIOS / ACPI ensures that. The kernel driver just uses the ACPI
provided reset line.

>
> >       hdev->open   = btusb_open;
> >       hdev->close  = btusb_close;
> >       hdev->flush  = btusb_flush;
> > @@ -3085,6 +3119,7 @@ static int btusb_probe(struct usb_interface *intf,
> >               set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
> >               set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
> >               set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
> > +             set_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks);
>
> You are not messing with the quirks here please. Clearing quirks is crazy. Use the data->flags since this should be all btusb.c specific.

Do I understand it right that you do not want me to clear the quirks
in btusb_hw_reset() and use data->flags instead? Sure, I will do that.
But I'm not sure if that's all you meant, because you commented on
btusb_probe() code where I'm setting the quirk (not clearing it) so
that the hci core can call the hw_reset() callback on timeout.

Thanks,

Rajat
>
> >
> >               if (id->driver_info & BTUSB_INTEL) {
> >                       hdev->setup = btusb_setup_intel;
> > @@ -3225,6 +3260,8 @@ static int btusb_probe(struct usb_interface *intf,
> >       return 0;
> >
> > out_free_dev:
> > +     if (data->reset_gpio)
> > +             gpiod_put(data->reset_gpio);
> >       hci_free_dev(hdev);
> >       return err;
> > }
> > @@ -3268,6 +3305,9 @@ static void btusb_disconnect(struct usb_interface *intf)
> >       if (data->oob_wake_irq)
> >               device_init_wakeup(&data->udev->dev, false);
> >
> > +     if (data->reset_gpio)
> > +             gpiod_put(data->reset_gpio);
> > +
> >       hci_free_dev(hdev);
> > }
>
> Regards
>
> Marcel
>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* [PATCH v4 1/5] usb: split code locating ACPI companion into port and device
  2018-11-17  1:07 [PATCH 0/5] Reset Intel BT controller if it gets stuck Rajat Jain
                   ` (6 preceding siblings ...)
  2018-11-21 23:50 ` [PATCH v3 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
@ 2019-01-18 22:34 ` Rajat Jain
  2019-01-18 22:34   ` [PATCH v4 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
                     ` (4 more replies)
  2019-01-23 20:57 ` [PATCH v5 1/4] " Rajat Jain
  2019-01-24 23:28 ` [PATCH v6 1/4] usb: split code locating ACPI companion into port and device Rajat Jain
  9 siblings, 5 replies; 56+ messages in thread
From: Rajat Jain @ 2019-01-18 22:34 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

From: Dmitry Torokhov <dtor@chromium.org>

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 <dtor@chromium.org>
Signed-off-by: Rajat Jain <rajatja@google.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v4 2/5] usb: assign ACPI companions for embedded USB devices
  2019-01-18 22:34 ` [PATCH v4 " Rajat Jain
@ 2019-01-18 22:34   ` Rajat Jain
  2019-01-19 19:51     ` Marcel Holtmann
  2019-01-18 22:34   ` [PATCH v4 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts Rajat Jain
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 56+ messages in thread
From: Rajat Jain @ 2019-01-18 22:34 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

From: Dmitry Torokhov <dtor@chromium.org>

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 <dtor@chromium.org>
Signed-off-by: Rajat Jain <rajatja@google.com> (changed how we get the usb_port)
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v4 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts
  2019-01-18 22:34 ` [PATCH v4 " Rajat Jain
  2019-01-18 22:34   ` [PATCH v4 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
@ 2019-01-18 22:34   ` Rajat Jain
  2019-01-19 19:51     ` Marcel Holtmann
  2019-01-18 22:34   ` [PATCH v4 4/5] Bluetooth: btusb: Collect the common Intel assignments together Rajat Jain
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 56+ messages in thread
From: Rajat Jain @ 2019-01-18 22:34 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v4 4/5] Bluetooth: btusb: Collect the common Intel assignments together
  2019-01-18 22:34 ` [PATCH v4 " Rajat Jain
  2019-01-18 22:34   ` [PATCH v4 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
  2019-01-18 22:34   ` [PATCH v4 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts Rajat Jain
@ 2019-01-18 22:34   ` Rajat Jain
  2019-01-19 19:51     ` Marcel Holtmann
  2019-01-18 22:34   ` [PATCH v4 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip Rajat Jain
  2019-01-19 19:51   ` [PATCH v4 1/5] usb: split code locating ACPI companion into port and device Marcel Holtmann
  4 siblings, 1 reply; 56+ messages in thread
From: Rajat Jain @ 2019-01-18 22:34 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v4 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip
  2019-01-18 22:34 ` [PATCH v4 " Rajat Jain
                     ` (2 preceding siblings ...)
  2019-01-18 22:34   ` [PATCH v4 4/5] Bluetooth: btusb: Collect the common Intel assignments together Rajat Jain
@ 2019-01-18 22:34   ` Rajat Jain
  2019-01-19 19:51     ` Marcel Holtmann
  2019-01-19 19:51   ` [PATCH v4 1/5] usb: split code locating ACPI companion into port and device Marcel Holtmann
  4 siblings, 1 reply; 56+ messages in thread
From: Rajat Jain @ 2019-01-18 22:34 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

If the platform provides it, use the reset gpio to reset the BT
chip (requested by the HCI core if needed). This has been found helpful
on some of Intel bluetooth controllers where the firmware gets stuck and
the only way out is a hard reset pin provided by the platform.

Signed-off-by: Rajat Jain <rajatja@google.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* Re: [PATCH v4 1/5] usb: split code locating ACPI companion into port and device
  2019-01-18 22:34 ` [PATCH v4 " Rajat Jain
                     ` (3 preceding siblings ...)
  2019-01-18 22:34   ` [PATCH v4 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip Rajat Jain
@ 2019-01-19 19:51   ` Marcel Holtmann
  2019-01-22 22:28     ` Rajat Jain
  4 siblings, 1 reply; 56+ messages in thread
From: Marcel Holtmann @ 2019-01-19 19:51 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, Bluez mailing list,
	Linux Kernel Mailing List, linux-usb, netdev, rajatxjain, dtor,
	Raghuram Hegde, chethan.tumkur.narayan, sukumar.ghorai

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 <dtor@chromium.org>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
> ---
> 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


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v4 2/5] usb: assign ACPI companions for embedded USB devices
  2019-01-18 22:34   ` [PATCH v4 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
@ 2019-01-19 19:51     ` Marcel Holtmann
  2019-01-22 22:29       ` Rajat Jain
  0 siblings, 1 reply; 56+ messages in thread
From: Marcel Holtmann @ 2019-01-19 19:51 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, linux-bluetooth, linux-kernel,
	linux-usb, netdev, rajatxjain, dtor, raghuram.hegde,
	chethan.tumkur.narayan, sukumar.ghorai

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 <dtor@chromium.org>
> Signed-off-by: Rajat Jain <rajatja@google.com> (changed how we get the usb_port)
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
> ---
> 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


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v4 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts
  2019-01-18 22:34   ` [PATCH v4 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts Rajat Jain
@ 2019-01-19 19:51     ` Marcel Holtmann
  2019-01-22 22:34       ` Rajat Jain
  0 siblings, 1 reply; 56+ messages in thread
From: Marcel Holtmann @ 2019-01-19 19:51 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, linux-bluetooth, linux-kernel,
	linux-usb, netdev, rajatxjain, dtor, raghuram.hegde,
	chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
> ---
> 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


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v4 4/5] Bluetooth: btusb: Collect the common Intel assignments together
  2019-01-18 22:34   ` [PATCH v4 4/5] Bluetooth: btusb: Collect the common Intel assignments together Rajat Jain
@ 2019-01-19 19:51     ` Marcel Holtmann
  2019-01-22 22:35       ` Rajat Jain
  0 siblings, 1 reply; 56+ messages in thread
From: Marcel Holtmann @ 2019-01-19 19:51 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, linux-bluetooth, linux-kernel,
	linux-usb, netdev, rajatxjain, dtor, raghuram.hegde,
	chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
> ---
> 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


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v4 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip
  2019-01-18 22:34   ` [PATCH v4 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip Rajat Jain
@ 2019-01-19 19:51     ` Marcel Holtmann
  2019-01-22 22:36       ` Rajat Jain
  0 siblings, 1 reply; 56+ messages in thread
From: Marcel Holtmann @ 2019-01-19 19:51 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, linux-bluetooth, linux-kernel,
	linux-usb, netdev, rajatxjain, dtor, raghuram.hegde,
	chethan.tumkur.narayan, sukumar.ghorai

Hi Rajat,

> If the platform provides it, use the reset gpio to reset the BT
> chip (requested by the HCI core if needed). This has been found helpful
> on some of Intel bluetooth controllers where the firmware gets stuck and
> the only way out is a hard reset pin provided by the platform.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> 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


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v4 1/5] usb: split code locating ACPI companion into port and device
  2019-01-19 19:51   ` [PATCH v4 1/5] usb: split code locating ACPI companion into port and device Marcel Holtmann
@ 2019-01-22 22:28     ` Rajat Jain
  2019-01-22 22:42       ` Dmitry Torokhov
  0 siblings, 1 reply; 56+ messages in thread
From: Rajat Jain @ 2019-01-22 22:28 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, Bluez mailing list,
	Linux Kernel Mailing List, linux-usb, netdev, Rajat Jain,
	Dmitry Torokhov, Raghuram Hegde, chethan.tumkur.narayan, Ghorai,
	Sukumar

Hi Marcel,

On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <marcel@holtmann.org> 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 <dtor@chromium.org>
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
> > ---
> > 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
>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v4 2/5] usb: assign ACPI companions for embedded USB devices
  2019-01-19 19:51     ` Marcel Holtmann
@ 2019-01-22 22:29       ` Rajat Jain
  0 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2019-01-22 22:29 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, linux-bluetooth,
	Linux Kernel Mailing List, linux-usb, netdev, Rajat Jain,
	Dmitry Torokhov, raghuram.hegde, chethan.tumkur.narayan, Ghorai,
	Sukumar

On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <marcel@holtmann.org> 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 <dtor@chromium.org>
> > Signed-off-by: Rajat Jain <rajatja@google.com> (changed how we get the usb_port)
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
> > ---
> > 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
>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v4 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts
  2019-01-19 19:51     ` Marcel Holtmann
@ 2019-01-22 22:34       ` Rajat Jain
  0 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2019-01-22 22:34 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, Bluez mailing list,
	Linux Kernel Mailing List, linux-usb, netdev, Rajat Jain,
	Dmitry Torokhov, Raghuram Hegde, chethan.tumkur.narayan, Ghorai,
	Sukumar

On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <marcel@holtmann.org> 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 <rajatja@google.com>
> > ---
> > 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
>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v4 4/5] Bluetooth: btusb: Collect the common Intel assignments together
  2019-01-19 19:51     ` Marcel Holtmann
@ 2019-01-22 22:35       ` Rajat Jain
  0 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2019-01-22 22:35 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, Bluez mailing list,
	Linux Kernel Mailing List, linux-usb, netdev, Rajat Jain,
	Dmitry Torokhov, Raghuram Hegde, chethan.tumkur.narayan, Ghorai,
	Sukumar

On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <marcel@holtmann.org> 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 <rajatja@google.com>
> > ---
> > 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
>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v4 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip
  2019-01-19 19:51     ` Marcel Holtmann
@ 2019-01-22 22:36       ` Rajat Jain
  0 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2019-01-22 22:36 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, Bluez mailing list,
	Linux Kernel Mailing List, linux-usb, netdev, Rajat Jain,
	Dmitry Torokhov, Raghuram Hegde, chethan.tumkur.narayan, Ghorai,
	Sukumar

On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Rajat,
>
> > If the platform provides it, use the reset gpio to reset the BT
> > chip (requested by the HCI core if needed). This has been found helpful
> > on some of Intel bluetooth controllers where the firmware gets stuck and
> > the only way out is a hard reset pin provided by the platform.
> >
> > Signed-off-by: Rajat Jain <rajatja@google.com>
> > ---
> > 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

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v4 1/5] usb: split code locating ACPI companion into port and device
  2019-01-22 22:28     ` Rajat Jain
@ 2019-01-22 22:42       ` Dmitry Torokhov
  2019-01-23  6:36         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 56+ messages in thread
From: Dmitry Torokhov @ 2019-01-22 22:42 UTC (permalink / raw)
  To: Rajat Jain, Greg Kroah-Hartman
  Cc: Marcel Holtmann, Johan Hedberg, David S. Miller, Alex Hung,
	Bluez mailing list, Linux Kernel Mailing List, linux-usb, netdev,
	Rajat Jain, Raghuram Hegde, chethan.tumkur.narayan, Ghorai,
	Sukumar

On Tue, Jan 22, 2019 at 2:29 PM Rajat Jain <rajatja@google.com> wrote:
>
> Hi Marcel,
>
> On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <marcel@holtmann.org> 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 <dtor@chromium.org>
> > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
> > > ---
> > > 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

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v4 1/5] usb: split code locating ACPI companion into port and device
  2019-01-22 22:42       ` Dmitry Torokhov
@ 2019-01-23  6:36         ` Greg Kroah-Hartman
  0 siblings, 0 replies; 56+ messages in thread
From: Greg Kroah-Hartman @ 2019-01-23  6:36 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rajat Jain, Marcel Holtmann, Johan Hedberg, David S. Miller,
	Alex Hung, Bluez mailing list, Linux Kernel Mailing List,
	linux-usb, netdev, Rajat Jain, Raghuram Hegde,
	chethan.tumkur.narayan, Ghorai, Sukumar

On Tue, Jan 22, 2019 at 02:42:26PM -0800, Dmitry Torokhov wrote:
> On Tue, Jan 22, 2019 at 2:29 PM Rajat Jain <rajatja@google.com> wrote:
> >
> > Hi Marcel,
> >
> > On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <marcel@holtmann.org> 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 <dtor@chromium.org>
> > > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
> > > > ---
> > > > 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

^ permalink raw reply	[flat|nested] 56+ messages in thread

* [PATCH v5 1/4] usb: split code locating ACPI companion into port and device
  2018-11-17  1:07 [PATCH 0/5] Reset Intel BT controller if it gets stuck Rajat Jain
                   ` (7 preceding siblings ...)
  2019-01-18 22:34 ` [PATCH v4 " Rajat Jain
@ 2019-01-23 20:57 ` Rajat Jain
  2019-01-23 20:57   ` [PATCH v5 2/4] usb: assign ACPI companions for embedded USB devices Rajat Jain
                     ` (2 more replies)
  2019-01-24 23:28 ` [PATCH v6 1/4] usb: split code locating ACPI companion into port and device Rajat Jain
  9 siblings, 3 replies; 56+ messages in thread
From: Rajat Jain @ 2019-01-23 20:57 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

From: Dmitry Torokhov <dtor@chromium.org>

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 <dtor@chromium.org>
Signed-off-by: Rajat Jain <rajatja@google.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v5 2/4] usb: assign ACPI companions for embedded USB devices
  2019-01-23 20:57 ` [PATCH v5 1/4] " Rajat Jain
@ 2019-01-23 20:57   ` Rajat Jain
  2019-01-23 20:57   ` [PATCH v5 3/4] Bluetooth: Allow driver specific cmd timeout handling Rajat Jain
  2019-01-23 20:57   ` [PATCH v5 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip Rajat Jain
  2 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2019-01-23 20:57 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

From: Dmitry Torokhov <dtor@chromium.org>

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 <dtor@chromium.org>
Signed-off-by: Rajat Jain <rajatja@google.com> (changed how we get the usb_port)
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v5 3/4] Bluetooth: Allow driver specific cmd timeout handling
  2019-01-23 20:57 ` [PATCH v5 1/4] " Rajat Jain
  2019-01-23 20:57   ` [PATCH v5 2/4] usb: assign ACPI companions for embedded USB devices Rajat Jain
@ 2019-01-23 20:57   ` Rajat Jain
  2019-01-24  9:36     ` Marcel Holtmann
  2019-01-23 20:57   ` [PATCH v5 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip Rajat Jain
  2 siblings, 1 reply; 56+ messages in thread
From: Rajat Jain @ 2019-01-23 20:57 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v5 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip
  2019-01-23 20:57 ` [PATCH v5 1/4] " Rajat Jain
  2019-01-23 20:57   ` [PATCH v5 2/4] usb: assign ACPI companions for embedded USB devices Rajat Jain
  2019-01-23 20:57   ` [PATCH v5 3/4] Bluetooth: Allow driver specific cmd timeout handling Rajat Jain
@ 2019-01-23 20:57   ` Rajat Jain
  2019-01-24  9:46     ` Marcel Holtmann
  2 siblings, 1 reply; 56+ messages in thread
From: Rajat Jain @ 2019-01-23 20:57 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* Re: [PATCH v5 3/4] Bluetooth: Allow driver specific cmd timeout handling
  2019-01-23 20:57   ` [PATCH v5 3/4] Bluetooth: Allow driver specific cmd timeout handling Rajat Jain
@ 2019-01-24  9:36     ` Marcel Holtmann
  2019-01-24 20:10       ` Rajat Jain
  0 siblings, 1 reply; 56+ messages in thread
From: Marcel Holtmann @ 2019-01-24  9:36 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, Bluez mailing list,
	Linux Kernel Mailing List, linux-usb, netdev, rajatxjain, dtor,
	Raghuram Hegde, chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
> ---
> 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


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v5 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip
  2019-01-23 20:57   ` [PATCH v5 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip Rajat Jain
@ 2019-01-24  9:46     ` Marcel Holtmann
  2019-01-24 20:05       ` Rajat Jain
  0 siblings, 1 reply; 56+ messages in thread
From: Marcel Holtmann @ 2019-01-24  9:46 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, linux-bluetooth, linux-kernel,
	linux-usb, netdev, rajatxjain, dtor, raghuram.hegde,
	chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
> ---
> 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


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v5 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip
  2019-01-24  9:46     ` Marcel Holtmann
@ 2019-01-24 20:05       ` Rajat Jain
  0 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2019-01-24 20:05 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, Bluez mailing list,
	Linux Kernel Mailing List, linux-usb, netdev, Rajat Jain,
	Dmitry Torokhov, Raghuram Hegde, chethan.tumkur.narayan, Ghorai,
	Sukumar

On Thu, Jan 24, 2019 at 1:46 AM Marcel Holtmann <marcel@holtmann.org> 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 <rajatja@google.com>
> > ---
> > 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
>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v5 3/4] Bluetooth: Allow driver specific cmd timeout handling
  2019-01-24  9:36     ` Marcel Holtmann
@ 2019-01-24 20:10       ` Rajat Jain
  0 siblings, 0 replies; 56+ messages in thread
From: Rajat Jain @ 2019-01-24 20:10 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, Bluez mailing list,
	Linux Kernel Mailing List, linux-usb, netdev, Rajat Jain,
	Dmitry Torokhov, Raghuram Hegde, chethan.tumkur.narayan, Ghorai,
	Sukumar

Hi Marcel,

On Thu, Jan 24, 2019 at 1:36 AM Marcel Holtmann <marcel@holtmann.org> 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 <rajatja@google.com>
> > ---
> > 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
>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* [PATCH v6 1/4] usb: split code locating ACPI companion into port and device
  2018-11-17  1:07 [PATCH 0/5] Reset Intel BT controller if it gets stuck Rajat Jain
                   ` (8 preceding siblings ...)
  2019-01-23 20:57 ` [PATCH v5 1/4] " Rajat Jain
@ 2019-01-24 23:28 ` Rajat Jain
  2019-01-24 23:28   ` [PATCH v6 2/4] usb: assign ACPI companions for embedded USB devices Rajat Jain
                     ` (3 more replies)
  9 siblings, 4 replies; 56+ messages in thread
From: Rajat Jain @ 2019-01-24 23:28 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

From: Dmitry Torokhov <dtor@chromium.org>

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 <dtor@chromium.org>
Signed-off-by: Rajat Jain <rajatja@google.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v6 2/4] usb: assign ACPI companions for embedded USB devices
  2019-01-24 23:28 ` [PATCH v6 1/4] usb: split code locating ACPI companion into port and device Rajat Jain
@ 2019-01-24 23:28   ` Rajat Jain
  2019-01-25  7:51     ` Marcel Holtmann
  2019-01-24 23:28   ` [PATCH v6 3/4] Bluetooth: Allow driver specific cmd timeout handling Rajat Jain
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 56+ messages in thread
From: Rajat Jain @ 2019-01-24 23:28 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

From: Dmitry Torokhov <dtor@chromium.org>

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 <dtor@chromium.org>
Signed-off-by: Rajat Jain <rajatja@google.com> (changed how we get the usb_port)
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v6 3/4] Bluetooth: Allow driver specific cmd timeout handling
  2019-01-24 23:28 ` [PATCH v6 1/4] usb: split code locating ACPI companion into port and device Rajat Jain
  2019-01-24 23:28   ` [PATCH v6 2/4] usb: assign ACPI companions for embedded USB devices Rajat Jain
@ 2019-01-24 23:28   ` Rajat Jain
  2019-01-25  7:51     ` Marcel Holtmann
  2019-01-24 23:28   ` [PATCH v6 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip Rajat Jain
  2019-01-25  7:51   ` [PATCH v6 1/4] usb: split code locating ACPI companion into port and device Marcel Holtmann
  3 siblings, 1 reply; 56+ messages in thread
From: Rajat Jain @ 2019-01-24 23:28 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [PATCH v6 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip
  2019-01-24 23:28 ` [PATCH v6 1/4] usb: split code locating ACPI companion into port and device Rajat Jain
  2019-01-24 23:28   ` [PATCH v6 2/4] usb: assign ACPI companions for embedded USB devices Rajat Jain
  2019-01-24 23:28   ` [PATCH v6 3/4] Bluetooth: Allow driver specific cmd timeout handling Rajat Jain
@ 2019-01-24 23:28   ` Rajat Jain
  2019-01-25  7:51     ` Marcel Holtmann
  2019-01-25  7:51   ` [PATCH v6 1/4] usb: split code locating ACPI companion into port and device Marcel Holtmann
  3 siblings, 1 reply; 56+ messages in thread
From: Rajat Jain @ 2019-01-24 23:28 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Greg Kroah-Hartman,
	David S. Miller, Dmitry Torokhov, Rajat Jain, Alex Hung,
	linux-bluetooth, linux-kernel, linux-usb, netdev
  Cc: rajatxjain, dtor, raghuram.hegde, chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
---
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


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* Re: [PATCH v6 1/4] usb: split code locating ACPI companion into port and device
  2019-01-24 23:28 ` [PATCH v6 1/4] usb: split code locating ACPI companion into port and device Rajat Jain
                     ` (2 preceding siblings ...)
  2019-01-24 23:28   ` [PATCH v6 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip Rajat Jain
@ 2019-01-25  7:51   ` Marcel Holtmann
  3 siblings, 0 replies; 56+ messages in thread
From: Marcel Holtmann @ 2019-01-25  7:51 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, Bluez mailing list,
	Linux Kernel Mailing List, linux-usb, netdev, rajatxjain, dtor,
	Raghuram Hegde, Chethan T N, sukumar.ghorai

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 <dtor@chromium.org>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
> ---
> 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


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v6 2/4] usb: assign ACPI companions for embedded USB devices
  2019-01-24 23:28   ` [PATCH v6 2/4] usb: assign ACPI companions for embedded USB devices Rajat Jain
@ 2019-01-25  7:51     ` Marcel Holtmann
  0 siblings, 0 replies; 56+ messages in thread
From: Marcel Holtmann @ 2019-01-25  7:51 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, linux-bluetooth, linux-kernel,
	linux-usb, netdev, rajatxjain, dtor, raghuram.hegde,
	chethan.tumkur.narayan, sukumar.ghorai

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 <dtor@chromium.org>
> Signed-off-by: Rajat Jain <rajatja@google.com> (changed how we get the usb_port)
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Tested-by: Sukumar Ghorai <sukumar.ghorai@intel.com>
> ---
> 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


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v6 3/4] Bluetooth: Allow driver specific cmd timeout handling
  2019-01-24 23:28   ` [PATCH v6 3/4] Bluetooth: Allow driver specific cmd timeout handling Rajat Jain
@ 2019-01-25  7:51     ` Marcel Holtmann
  0 siblings, 0 replies; 56+ messages in thread
From: Marcel Holtmann @ 2019-01-25  7:51 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, linux-bluetooth, linux-kernel,
	linux-usb, netdev, rajatxjain, dtor, raghuram.hegde,
	chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
> ---
> 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


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [PATCH v6 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip
  2019-01-24 23:28   ` [PATCH v6 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip Rajat Jain
@ 2019-01-25  7:51     ` Marcel Holtmann
  0 siblings, 0 replies; 56+ messages in thread
From: Marcel Holtmann @ 2019-01-25  7:51 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Johan Hedberg, Greg Kroah-Hartman, David S. Miller,
	Dmitry Torokhov, Alex Hung, linux-bluetooth, linux-kernel,
	linux-usb, netdev, rajatxjain, dtor, raghuram.hegde,
	chethan.tumkur.narayan, sukumar.ghorai

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 <rajatja@google.com>
> ---
> 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


^ permalink raw reply	[flat|nested] 56+ messages in thread

end of thread, other threads:[~2019-01-25  7:51 UTC | newest]

Thread overview: 56+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-17  1:07 [PATCH 0/5] Reset Intel BT controller if it gets stuck Rajat Jain
2018-11-17  1:07 ` [PATCH 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
2018-11-17  1:07 ` [PATCH 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
2018-11-17  1:07 ` [PATCH 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts Rajat Jain
2018-11-17  1:07 ` [PATCH 4/5] Bluetooth: btusb: Collect the common Intel assignments together Rajat Jain
2018-11-17  1:07 ` [PATCH 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip Rajat Jain
2018-11-19 23:04 ` [PATCH v2 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
2018-11-19 23:04   ` [PATCH v2 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
2018-11-19 23:04   ` [PATCH v2 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts Rajat Jain
2018-11-19 23:04   ` [PATCH v2 4/5] Bluetooth: btusb: Collect the common Intel assignments together Rajat Jain
2018-11-19 23:04   ` [PATCH v2 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip Rajat Jain
2018-11-21 23:50 ` [PATCH v3 1/5] usb: split code locating ACPI companion into port and device Rajat Jain
2018-11-21 23:50   ` [PATCH v3 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
2018-12-05  9:32     ` Greg Kroah-Hartman
2018-12-05 17:19       ` Ghorai, Sukumar
2018-11-21 23:50   ` [PATCH v3 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts Rajat Jain
2018-11-21 23:50   ` [PATCH v3 4/5] Bluetooth: btusb: Collect the common Intel assignments together Rajat Jain
2018-11-21 23:50   ` [PATCH v3 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip Rajat Jain
2018-12-20  8:46     ` Rajat Jain
2019-01-18 11:04     ` Marcel Holtmann
2019-01-18 20:51       ` Rajat Jain
2018-12-05  9:32   ` [PATCH v3 1/5] usb: split code locating ACPI companion into port and device Greg Kroah-Hartman
2018-12-05 17:41     ` Ghorai, Sukumar
2019-01-18 22:34 ` [PATCH v4 " Rajat Jain
2019-01-18 22:34   ` [PATCH v4 2/5] usb: assign ACPI companions for embedded USB devices Rajat Jain
2019-01-19 19:51     ` Marcel Holtmann
2019-01-22 22:29       ` Rajat Jain
2019-01-18 22:34   ` [PATCH v4 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts Rajat Jain
2019-01-19 19:51     ` Marcel Holtmann
2019-01-22 22:34       ` Rajat Jain
2019-01-18 22:34   ` [PATCH v4 4/5] Bluetooth: btusb: Collect the common Intel assignments together Rajat Jain
2019-01-19 19:51     ` Marcel Holtmann
2019-01-22 22:35       ` Rajat Jain
2019-01-18 22:34   ` [PATCH v4 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip Rajat Jain
2019-01-19 19:51     ` Marcel Holtmann
2019-01-22 22:36       ` Rajat Jain
2019-01-19 19:51   ` [PATCH v4 1/5] usb: split code locating ACPI companion into port and device Marcel Holtmann
2019-01-22 22:28     ` Rajat Jain
2019-01-22 22:42       ` Dmitry Torokhov
2019-01-23  6:36         ` Greg Kroah-Hartman
2019-01-23 20:57 ` [PATCH v5 1/4] " Rajat Jain
2019-01-23 20:57   ` [PATCH v5 2/4] usb: assign ACPI companions for embedded USB devices Rajat Jain
2019-01-23 20:57   ` [PATCH v5 3/4] Bluetooth: Allow driver specific cmd timeout handling Rajat Jain
2019-01-24  9:36     ` Marcel Holtmann
2019-01-24 20:10       ` Rajat Jain
2019-01-23 20:57   ` [PATCH v5 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip Rajat Jain
2019-01-24  9:46     ` Marcel Holtmann
2019-01-24 20:05       ` Rajat Jain
2019-01-24 23:28 ` [PATCH v6 1/4] usb: split code locating ACPI companion into port and device Rajat Jain
2019-01-24 23:28   ` [PATCH v6 2/4] usb: assign ACPI companions for embedded USB devices Rajat Jain
2019-01-25  7:51     ` Marcel Holtmann
2019-01-24 23:28   ` [PATCH v6 3/4] Bluetooth: Allow driver specific cmd timeout handling Rajat Jain
2019-01-25  7:51     ` Marcel Holtmann
2019-01-24 23:28   ` [PATCH v6 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip Rajat Jain
2019-01-25  7:51     ` Marcel Holtmann
2019-01-25  7:51   ` [PATCH v6 1/4] usb: split code locating ACPI companion into port and device Marcel Holtmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).