linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag
@ 2021-07-15 14:51 Archie Pusaka
  2021-07-15 14:51 ` [PATCH v2 2/3] Bluetooth: hci_h5: btrtl: Maintain flow control if wakeup is enabled Archie Pusaka
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Archie Pusaka @ 2021-07-15 14:51 UTC (permalink / raw)
  To: linux-bluetooth, Marcel Holtmann
  Cc: CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi,
	Hilda Wu, Johan Hedberg, Luiz Augusto von Dentz, linux-kernel

From: Archie Pusaka <apusaka@chromium.org>

Some RTL chips resets the FW on suspend, so wakeup is disabled on
those chips. This patch introduces this WAKEUP_DISABLE flag so that
chips that doesn't reset FW on suspend can leave the flag unset and
is allowed to wake the host.

This patch also left RTL8822 WAKEUP_DISABLE flag unset, therefore
allowing it to wake the host, and preventing reprobing on resume.

Signed-off-by: Archie Pusaka <apusaka@chromium.org>
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Hilda Wu <hildawu@realtek.com>

---

Changes in v2:
* Remove unnecessary variable

 drivers/bluetooth/hci_h5.c | 83 +++++++++++++++++++++++++++-----------
 1 file changed, 59 insertions(+), 24 deletions(-)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index 7b985c7cd26d..fd672111a048 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -51,8 +51,9 @@
 
 /* H5 state flags */
 enum {
-	H5_RX_ESC,	/* SLIP escape mode */
-	H5_TX_ACK_REQ,	/* Pending ack to send */
+	H5_RX_ESC,		/* SLIP escape mode */
+	H5_TX_ACK_REQ,		/* Pending ack to send */
+	H5_WAKEUP_DISABLE,	/* Device cannot wake host */
 };
 
 struct h5 {
@@ -97,6 +98,10 @@ struct h5 {
 	struct gpio_desc *device_wake_gpio;
 };
 
+enum h5_driver_info {
+	H5_INFO_WAKEUP_DISABLE = BIT(0),
+};
+
 struct h5_vnd {
 	int (*setup)(struct h5 *h5);
 	void (*open)(struct h5 *h5);
@@ -106,6 +111,11 @@ struct h5_vnd {
 	const struct acpi_gpio_mapping *acpi_gpio_map;
 };
 
+struct h5_device_data {
+	uint32_t driver_info;
+	struct h5_vnd *vnd;
+};
+
 static void h5_reset_rx(struct h5 *h5);
 
 static void h5_link_control(struct hci_uart *hu, const void *data, size_t len)
@@ -791,6 +801,8 @@ static int h5_serdev_probe(struct serdev_device *serdev)
 {
 	struct device *dev = &serdev->dev;
 	struct h5 *h5;
+	const struct h5_device_data *data;
+	int err;
 
 	h5 = devm_kzalloc(dev, sizeof(*h5), GFP_KERNEL);
 	if (!h5)
@@ -807,20 +819,19 @@ static int h5_serdev_probe(struct serdev_device *serdev)
 		if (!match)
 			return -ENODEV;
 
-		h5->vnd = (const struct h5_vnd *)match->driver_data;
+		data = (const struct h5_device_data *)match->driver_data;
+		h5->vnd = data->vnd;
 		h5->id  = (char *)match->id;
 
 		if (h5->vnd->acpi_gpio_map)
 			devm_acpi_dev_add_driver_gpios(dev,
 						       h5->vnd->acpi_gpio_map);
 	} else {
-		const void *data;
-
 		data = of_device_get_match_data(dev);
 		if (!data)
 			return -ENODEV;
 
-		h5->vnd = (const struct h5_vnd *)data;
+		h5->vnd = data->vnd;
 	}
 
 
@@ -833,7 +844,14 @@ static int h5_serdev_probe(struct serdev_device *serdev)
 	if (IS_ERR(h5->device_wake_gpio))
 		return PTR_ERR(h5->device_wake_gpio);
 
-	return hci_uart_register_device(&h5->serdev_hu, &h5p);
+	err = hci_uart_register_device(&h5->serdev_hu, &h5p);
+	if (err)
+		return err;
+
+	if (data->driver_info & H5_INFO_WAKEUP_DISABLE)
+		set_bit(H5_WAKEUP_DISABLE, &h5->flags);
+
+	return 0;
 }
 
 static void h5_serdev_remove(struct serdev_device *serdev)
@@ -921,7 +939,8 @@ static void h5_btrtl_open(struct h5 *h5)
 	 * done by the hci_suspend_notifier is not necessary; it actually causes
 	 * delays and a bunch of errors to get logged, so disable it.
 	 */
-	set_bit(HCI_UART_NO_SUSPEND_NOTIFIER, &h5->hu->flags);
+	if (test_bit(H5_WAKEUP_DISABLE, &h5->flags))
+		set_bit(HCI_UART_NO_SUSPEND_NOTIFIER, &h5->hu->flags);
 
 	/* Devices always start with these fixed parameters */
 	serdev_device_set_flow_control(h5->hu->serdev, false);
@@ -942,15 +961,18 @@ static void h5_btrtl_close(struct h5 *h5)
 
 /* Suspend/resume support. On many devices the RTL BT device loses power during
  * suspend/resume, causing it to lose its firmware and all state. So we simply
- * turn it off on suspend and reprobe on resume.  This mirrors how RTL devices
- * are handled in the USB driver, where the USB_QUIRK_RESET_RESUME is used which
+ * turn it off on suspend and reprobe on resume. This mirrors how RTL devices
+ * are handled in the USB driver, where the BTUSB_WAKEUP_DISABLE is used which
  * also causes a reprobe on resume.
  */
 static int h5_btrtl_suspend(struct h5 *h5)
 {
 	serdev_device_set_flow_control(h5->hu->serdev, false);
 	gpiod_set_value_cansleep(h5->device_wake_gpio, 0);
-	gpiod_set_value_cansleep(h5->enable_gpio, 0);
+
+	if (test_bit(H5_WAKEUP_DISABLE, &h5->flags))
+		gpiod_set_value_cansleep(h5->enable_gpio, 0);
+
 	return 0;
 }
 
@@ -976,17 +998,21 @@ static void h5_btrtl_reprobe_worker(struct work_struct *work)
 
 static int h5_btrtl_resume(struct h5 *h5)
 {
-	struct h5_btrtl_reprobe *reprobe;
+	if (test_bit(H5_WAKEUP_DISABLE, &h5->flags)) {
+		struct h5_btrtl_reprobe *reprobe;
 
-	reprobe = kzalloc(sizeof(*reprobe), GFP_KERNEL);
-	if (!reprobe)
-		return -ENOMEM;
+		reprobe = kzalloc(sizeof(*reprobe), GFP_KERNEL);
+		if (!reprobe)
+			return -ENOMEM;
 
-	__module_get(THIS_MODULE);
+		__module_get(THIS_MODULE);
 
-	INIT_WORK(&reprobe->work, h5_btrtl_reprobe_worker);
-	reprobe->dev = get_device(&h5->hu->serdev->dev);
-	queue_work(system_long_wq, &reprobe->work);
+		INIT_WORK(&reprobe->work, h5_btrtl_reprobe_worker);
+		reprobe->dev = get_device(&h5->hu->serdev->dev);
+		queue_work(system_long_wq, &reprobe->work);
+	} else {
+		gpiod_set_value_cansleep(h5->device_wake_gpio, 1);
+	}
 	return 0;
 }
 
@@ -1008,13 +1034,22 @@ static struct h5_vnd rtl_vnd = {
 	.resume		= h5_btrtl_resume,
 	.acpi_gpio_map	= acpi_btrtl_gpios,
 };
+
+static const struct h5_device_data h5_data_rtl8822cs = {
+	.vnd = &rtl_vnd,
+};
+
+static const struct h5_device_data h5_data_rtl8723bs = {
+	.driver_info = H5_INFO_WAKEUP_DISABLE,
+	.vnd = &rtl_vnd,
+};
 #endif
 
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id h5_acpi_match[] = {
 #ifdef CONFIG_BT_HCIUART_RTL
-	{ "OBDA0623", (kernel_ulong_t)&rtl_vnd },
-	{ "OBDA8723", (kernel_ulong_t)&rtl_vnd },
+	{ "OBDA0623", (kernel_ulong_t)&h5_data_rtl8723bs },
+	{ "OBDA8723", (kernel_ulong_t)&h5_data_rtl8723bs },
 #endif
 	{ },
 };
@@ -1028,11 +1063,11 @@ static const struct dev_pm_ops h5_serdev_pm_ops = {
 static const struct of_device_id rtl_bluetooth_of_match[] = {
 #ifdef CONFIG_BT_HCIUART_RTL
 	{ .compatible = "realtek,rtl8822cs-bt",
-	  .data = (const void *)&rtl_vnd },
+	  .data = (const void *)&h5_data_rtl8822cs },
 	{ .compatible = "realtek,rtl8723bs-bt",
-	  .data = (const void *)&rtl_vnd },
+	  .data = (const void *)&h5_data_rtl8723bs },
 	{ .compatible = "realtek,rtl8723ds-bt",
-	  .data = (const void *)&rtl_vnd },
+	  .data = (const void *)&h5_data_rtl8723bs },
 #endif
 	{ },
 };
-- 
2.32.0.93.g670b81a890-goog


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

* [PATCH v2 2/3] Bluetooth: hci_h5: btrtl: Maintain flow control if wakeup is enabled
  2021-07-15 14:51 [PATCH v2 1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag Archie Pusaka
@ 2021-07-15 14:51 ` Archie Pusaka
  2021-07-15 14:52 ` [PATCH v2 3/3] Bluetooth: hci_h5: Add runtime suspend Archie Pusaka
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Archie Pusaka @ 2021-07-15 14:51 UTC (permalink / raw)
  To: linux-bluetooth, Marcel Holtmann
  Cc: CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi,
	Hilda Wu, Johan Hedberg, Luiz Augusto von Dentz, linux-kernel

From: Archie Pusaka <apusaka@chromium.org>

For chips that doesn't reset on suspend, we need to provide the correct
value of flow_control when it resumes. Therefore, store the flow
control value when reading from the config file to be reused upon
suspend.

Signed-off-by: Archie Pusaka <apusaka@chromium.org>
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Hilda Wu <hildawu@realtek.com>

---

(no changes since v1)

 drivers/bluetooth/hci_h5.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index fd672111a048..cbc63b057f33 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -54,6 +54,7 @@ enum {
 	H5_RX_ESC,		/* SLIP escape mode */
 	H5_TX_ACK_REQ,		/* Pending ack to send */
 	H5_WAKEUP_DISABLE,	/* Device cannot wake host */
+	H5_HW_FLOW_CONTROL,	/* Use HW flow control */
 };
 
 struct h5 {
@@ -920,6 +921,9 @@ static int h5_btrtl_setup(struct h5 *h5)
 	serdev_device_set_baudrate(h5->hu->serdev, controller_baudrate);
 	serdev_device_set_flow_control(h5->hu->serdev, flow_control);
 
+	if (flow_control)
+		set_bit(H5_HW_FLOW_CONTROL, &h5->flags);
+
 	err = btrtl_download_firmware(h5->hu->hdev, btrtl_dev);
 	/* Give the device some time before the hci-core sends it a reset */
 	usleep_range(10000, 20000);
@@ -1012,7 +1016,11 @@ static int h5_btrtl_resume(struct h5 *h5)
 		queue_work(system_long_wq, &reprobe->work);
 	} else {
 		gpiod_set_value_cansleep(h5->device_wake_gpio, 1);
+
+		if (test_bit(H5_HW_FLOW_CONTROL, &h5->flags))
+			serdev_device_set_flow_control(h5->hu->serdev, true);
 	}
+
 	return 0;
 }
 
-- 
2.32.0.93.g670b81a890-goog


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

* [PATCH v2 3/3] Bluetooth: hci_h5: Add runtime suspend
  2021-07-15 14:51 [PATCH v2 1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag Archie Pusaka
  2021-07-15 14:51 ` [PATCH v2 2/3] Bluetooth: hci_h5: btrtl: Maintain flow control if wakeup is enabled Archie Pusaka
@ 2021-07-15 14:52 ` Archie Pusaka
  2021-07-15 16:15 ` [v2,1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag bluez.test.bot
  2021-07-22 14:32 ` [PATCH v2 1/3] " Marcel Holtmann
  3 siblings, 0 replies; 9+ messages in thread
From: Archie Pusaka @ 2021-07-15 14:52 UTC (permalink / raw)
  To: linux-bluetooth, Marcel Holtmann
  Cc: CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi,
	Hilda Wu, Johan Hedberg, Luiz Augusto von Dentz, linux-kernel

From: Archie Pusaka <apusaka@chromium.org>

This patch allows the controller to suspend after a short period of
inactivity.

Signed-off-by: Archie Pusaka <apusaka@chromium.org>
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Hilda Wu <hildawu@realtek.com>

---

(no changes since v1)

 drivers/bluetooth/hci_h5.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index cbc63b057f33..f95ec9991180 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -11,6 +11,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/mod_devicetable.h>
+#include <linux/pm_runtime.h>
 #include <linux/of_device.h>
 #include <linux/serdev.h>
 #include <linux/skbuff.h>
@@ -21,6 +22,8 @@
 #include "btrtl.h"
 #include "hci_uart.h"
 
+#define SUSPEND_TIMEOUT_MS	6000
+
 #define HCI_3WIRE_ACK_PKT	0
 #define HCI_3WIRE_LINK_PKT	15
 
@@ -584,6 +587,10 @@ static int h5_recv(struct hci_uart *hu, const void *data, int count)
 		count -= processed;
 	}
 
+	pm_runtime_get(&hu->serdev->dev);
+	pm_runtime_mark_last_busy(&hu->serdev->dev);
+	pm_runtime_put_autosuspend(&hu->serdev->dev);
+
 	return 0;
 }
 
@@ -620,6 +627,10 @@ static int h5_enqueue(struct hci_uart *hu, struct sk_buff *skb)
 		break;
 	}
 
+	pm_runtime_get_sync(&hu->serdev->dev);
+	pm_runtime_mark_last_busy(&hu->serdev->dev);
+	pm_runtime_put_autosuspend(&hu->serdev->dev);
+
 	return 0;
 }
 
@@ -951,6 +962,12 @@ static void h5_btrtl_open(struct h5 *h5)
 	serdev_device_set_parity(h5->hu->serdev, SERDEV_PARITY_EVEN);
 	serdev_device_set_baudrate(h5->hu->serdev, 115200);
 
+	pm_runtime_set_active(&h5->hu->serdev->dev);
+	pm_runtime_use_autosuspend(&h5->hu->serdev->dev);
+	pm_runtime_set_autosuspend_delay(&h5->hu->serdev->dev,
+					 SUSPEND_TIMEOUT_MS);
+	pm_runtime_enable(&h5->hu->serdev->dev);
+
 	/* The controller needs up to 500ms to wakeup */
 	gpiod_set_value_cansleep(h5->enable_gpio, 1);
 	gpiod_set_value_cansleep(h5->device_wake_gpio, 1);
@@ -959,6 +976,8 @@ static void h5_btrtl_open(struct h5 *h5)
 
 static void h5_btrtl_close(struct h5 *h5)
 {
+	pm_runtime_disable(&h5->hu->serdev->dev);
+
 	gpiod_set_value_cansleep(h5->device_wake_gpio, 0);
 	gpiod_set_value_cansleep(h5->enable_gpio, 0);
 }
@@ -1066,6 +1085,7 @@ MODULE_DEVICE_TABLE(acpi, h5_acpi_match);
 
 static const struct dev_pm_ops h5_serdev_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(h5_serdev_suspend, h5_serdev_resume)
+	SET_RUNTIME_PM_OPS(h5_serdev_suspend, h5_serdev_resume, NULL)
 };
 
 static const struct of_device_id rtl_bluetooth_of_match[] = {
-- 
2.32.0.93.g670b81a890-goog


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

* RE: [v2,1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag
  2021-07-15 14:51 [PATCH v2 1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag Archie Pusaka
  2021-07-15 14:51 ` [PATCH v2 2/3] Bluetooth: hci_h5: btrtl: Maintain flow control if wakeup is enabled Archie Pusaka
  2021-07-15 14:52 ` [PATCH v2 3/3] Bluetooth: hci_h5: Add runtime suspend Archie Pusaka
@ 2021-07-15 16:15 ` bluez.test.bot
  2021-07-22 14:32 ` [PATCH v2 1/3] " Marcel Holtmann
  3 siblings, 0 replies; 9+ messages in thread
From: bluez.test.bot @ 2021-07-15 16:15 UTC (permalink / raw)
  To: linux-bluetooth, apusaka

[-- Attachment #1: Type: text/plain, Size: 2727 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=516205

---Test result---

Test Summary:
CheckPatch                    PASS      1.37 seconds
GitLint                       PASS      0.35 seconds
BuildKernel                   PASS      639.76 seconds
TestRunner: Setup             PASS      429.36 seconds
TestRunner: l2cap-tester      PASS      3.14 seconds
TestRunner: bnep-tester       PASS      2.31 seconds
TestRunner: mgmt-tester       PASS      33.99 seconds
TestRunner: rfcomm-tester     PASS      2.44 seconds
TestRunner: sco-tester        PASS      2.36 seconds
TestRunner: smp-tester        FAIL      2.47 seconds
TestRunner: userchan-tester   PASS      2.28 seconds

Details
##############################
Test: CheckPatch - PASS - 1.37 seconds
Run checkpatch.pl script with rule in .checkpatch.conf


##############################
Test: GitLint - PASS - 0.35 seconds
Run gitlint with rule in .gitlint


##############################
Test: BuildKernel - PASS - 639.76 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 429.36 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 3.14 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 2.31 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - PASS - 33.99 seconds
Run test-runner with mgmt-tester
Total: 446, Passed: 443 (99.3%), Failed: 0, Not Run: 3

##############################
Test: TestRunner: rfcomm-tester - PASS - 2.44 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 2.36 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - FAIL - 2.47 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0

Failed Test Cases
SMP Client - SC Request 2                            Failed       0.030 seconds

##############################
Test: TestRunner: userchan-tester - PASS - 2.28 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth


[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 44349 bytes --]

[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3556 bytes --]

[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 614434 bytes --]

[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11677 bytes --]

[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9912 bytes --]

[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11704 bytes --]

[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5453 bytes --]

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

* Re: [PATCH v2 1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag
  2021-07-15 14:51 [PATCH v2 1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag Archie Pusaka
                   ` (2 preceding siblings ...)
  2021-07-15 16:15 ` [v2,1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag bluez.test.bot
@ 2021-07-22 14:32 ` Marcel Holtmann
  2021-07-23 11:42   ` Archie Pusaka
  3 siblings, 1 reply; 9+ messages in thread
From: Marcel Holtmann @ 2021-07-22 14:32 UTC (permalink / raw)
  To: Archie Pusaka
  Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka,
	Abhishek Pandit-Subedi, Hilda Wu, Johan Hedberg,
	Luiz Augusto von Dentz, linux-kernel

Hi Archie,

> Some RTL chips resets the FW on suspend, so wakeup is disabled on
> those chips. This patch introduces this WAKEUP_DISABLE flag so that
> chips that doesn't reset FW on suspend can leave the flag unset and
> is allowed to wake the host.
> 
> This patch also left RTL8822 WAKEUP_DISABLE flag unset, therefore
> allowing it to wake the host, and preventing reprobing on resume.
> 
> Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Reviewed-by: Hilda Wu <hildawu@realtek.com>
> 
> ---
> 
> Changes in v2:
> * Remove unnecessary variable
> 
> drivers/bluetooth/hci_h5.c | 83 +++++++++++++++++++++++++++-----------
> 1 file changed, 59 insertions(+), 24 deletions(-)

so the set does not apply cleanly to bluetooth-next

Applying: Bluetooth: hci_h5: Add runtime suspend
error: patch failed: drivers/bluetooth/hci_h5.c:11
error: drivers/bluetooth/hci_h5.c: patch does not apply

And I am really close to not accepting any patches for hci_h5.c anymore. This thing turns into crazy hacking and nobody is taking my hint to redo this as clean H:5 3-Wire serdev standalone driver.

Regards

Marcel


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

* Re: [PATCH v2 1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag
  2021-07-22 14:32 ` [PATCH v2 1/3] " Marcel Holtmann
@ 2021-07-23 11:42   ` Archie Pusaka
  2021-07-23 12:17     ` Marcel Holtmann
  0 siblings, 1 reply; 9+ messages in thread
From: Archie Pusaka @ 2021-07-23 11:42 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka,
	Abhishek Pandit-Subedi, Hilda Wu, Johan Hedberg,
	Luiz Augusto von Dentz, linux-kernel

Hi Marcel,

On Thu, 22 Jul 2021 at 22:32, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Archie,
>
> > Some RTL chips resets the FW on suspend, so wakeup is disabled on
> > those chips. This patch introduces this WAKEUP_DISABLE flag so that
> > chips that doesn't reset FW on suspend can leave the flag unset and
> > is allowed to wake the host.
> >
> > This patch also left RTL8822 WAKEUP_DISABLE flag unset, therefore
> > allowing it to wake the host, and preventing reprobing on resume.
> >
> > Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > Reviewed-by: Hilda Wu <hildawu@realtek.com>
> >
> > ---
> >
> > Changes in v2:
> > * Remove unnecessary variable
> >
> > drivers/bluetooth/hci_h5.c | 83 +++++++++++++++++++++++++++-----------
> > 1 file changed, 59 insertions(+), 24 deletions(-)
>
> so the set does not apply cleanly to bluetooth-next
>
> Applying: Bluetooth: hci_h5: Add runtime suspend
> error: patch failed: drivers/bluetooth/hci_h5.c:11
> error: drivers/bluetooth/hci_h5.c: patch does not apply

Hmm, it applies cleanly for me. Not sure what's going on.
Anyway I rebased and made a little change as v3, please take a look!

>
>
> And I am really close to not accepting any patches for hci_h5.c anymore. This thing turns into crazy hacking and nobody is taking my hint to redo this as clean H:5 3-Wire serdev standalone driver.

Pardon my unfamiliarity, but could you share more about your vision of
a clean h5 driver? Should the RTL component be moved out to btrtl?
Do we have something as a reference?

>
> Regards
>
> Marcel
>

Thanks,
Archie

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

* Re: [PATCH v2 1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag
  2021-07-23 11:42   ` Archie Pusaka
@ 2021-07-23 12:17     ` Marcel Holtmann
  2021-07-26  4:02       ` Archie Pusaka
  0 siblings, 1 reply; 9+ messages in thread
From: Marcel Holtmann @ 2021-07-23 12:17 UTC (permalink / raw)
  To: Archie Pusaka
  Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka,
	Abhishek Pandit-Subedi, Hilda Wu, Johan Hedberg,
	Luiz Augusto von Dentz, linux-kernel

Hi Archie,

>>> Some RTL chips resets the FW on suspend, so wakeup is disabled on
>>> those chips. This patch introduces this WAKEUP_DISABLE flag so that
>>> chips that doesn't reset FW on suspend can leave the flag unset and
>>> is allowed to wake the host.
>>> 
>>> This patch also left RTL8822 WAKEUP_DISABLE flag unset, therefore
>>> allowing it to wake the host, and preventing reprobing on resume.
>>> 
>>> Signed-off-by: Archie Pusaka <apusaka@chromium.org>
>>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>>> Reviewed-by: Hilda Wu <hildawu@realtek.com>
>>> 
>>> ---
>>> 
>>> Changes in v2:
>>> * Remove unnecessary variable
>>> 
>>> drivers/bluetooth/hci_h5.c | 83 +++++++++++++++++++++++++++-----------
>>> 1 file changed, 59 insertions(+), 24 deletions(-)
>> 
>> so the set does not apply cleanly to bluetooth-next
>> 
>> Applying: Bluetooth: hci_h5: Add runtime suspend
>> error: patch failed: drivers/bluetooth/hci_h5.c:11
>> error: drivers/bluetooth/hci_h5.c: patch does not apply
> 
> Hmm, it applies cleanly for me. Not sure what's going on.
> Anyway I rebased and made a little change as v3, please take a look!

the v3 applied cleanly.

>> 
>> 
>> And I am really close to not accepting any patches for hci_h5.c anymore. This thing turns into crazy hacking and nobody is taking my hint to redo this as clean H:5 3-Wire serdev standalone driver.
> 
> Pardon my unfamiliarity, but could you share more about your vision of
> a clean h5 driver? Should the RTL component be moved out to btrtl?
> Do we have something as a reference?

so a while back I send a bt3wire.c sample driver around. That would be a good starting point.

Anyhow, the problem is that hci_uart.ko is inherent a line discipline driver from 2.4.x kernel days and it has been stacked and hacked on top of it. It has become a burden, especially in the light that you can have clean serdev based drivers now (like btmtkuart.c).

And yes, it would be following the 3-Wire H:5 spec and then deal with vendor specific details like btusb.c for example. And my hope would be that especially in the Realtek and Broadcom (RPi3 etc.) cases this can move into vendor specific blocks and shared between USB and UART transports.

I also send around a btuart.c sample driver that is solely serdev based and should replace all the cases where we have H:4 as transport.

Regards

Marcel


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

* Re: [PATCH v2 1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag
  2021-07-23 12:17     ` Marcel Holtmann
@ 2021-07-26  4:02       ` Archie Pusaka
  2021-07-29 11:54         ` Marcel Holtmann
  0 siblings, 1 reply; 9+ messages in thread
From: Archie Pusaka @ 2021-07-26  4:02 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka,
	Abhishek Pandit-Subedi, Hilda Wu, Johan Hedberg,
	Luiz Augusto von Dentz, linux-kernel

Hi Marcel,

On Fri, 23 Jul 2021 at 20:17, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Archie,
>
> >>> Some RTL chips resets the FW on suspend, so wakeup is disabled on
> >>> those chips. This patch introduces this WAKEUP_DISABLE flag so that
> >>> chips that doesn't reset FW on suspend can leave the flag unset and
> >>> is allowed to wake the host.
> >>>
> >>> This patch also left RTL8822 WAKEUP_DISABLE flag unset, therefore
> >>> allowing it to wake the host, and preventing reprobing on resume.
> >>>
> >>> Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> >>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> >>> Reviewed-by: Hilda Wu <hildawu@realtek.com>
> >>>
> >>> ---
> >>>
> >>> Changes in v2:
> >>> * Remove unnecessary variable
> >>>
> >>> drivers/bluetooth/hci_h5.c | 83 +++++++++++++++++++++++++++-----------
> >>> 1 file changed, 59 insertions(+), 24 deletions(-)
> >>
> >> so the set does not apply cleanly to bluetooth-next
> >>
> >> Applying: Bluetooth: hci_h5: Add runtime suspend
> >> error: patch failed: drivers/bluetooth/hci_h5.c:11
> >> error: drivers/bluetooth/hci_h5.c: patch does not apply
> >
> > Hmm, it applies cleanly for me. Not sure what's going on.
> > Anyway I rebased and made a little change as v3, please take a look!
>
> the v3 applied cleanly.
>
> >>
> >>
> >> And I am really close to not accepting any patches for hci_h5.c anymore. This thing turns into crazy hacking and nobody is taking my hint to redo this as clean H:5 3-Wire serdev standalone driver.
> >
> > Pardon my unfamiliarity, but could you share more about your vision of
> > a clean h5 driver? Should the RTL component be moved out to btrtl?
> > Do we have something as a reference?
>
> so a while back I send a bt3wire.c sample driver around. That would be a good starting point.
>
> Anyhow, the problem is that hci_uart.ko is inherent a line discipline driver from 2.4.x kernel days and it has been stacked and hacked on top of it. It has become a burden, especially in the light that you can have clean serdev based drivers now (like btmtkuart.c).
>
> And yes, it would be following the 3-Wire H:5 spec and then deal with vendor specific details like btusb.c for example. And my hope would be that especially in the Realtek and Broadcom (RPi3 etc.) cases this can move into vendor specific blocks and shared between USB and UART transports.
>
> I also send around a btuart.c sample driver that is solely serdev based and should replace all the cases where we have H:4 as transport.
>

Thanks for the pointers!

The files you mentioned are rather hard to find, so below I paste the
URL where I found them in case anyone else is also interested.

[RFC v2] Bluetooth: Add new serdev based driver for UART attached controllers
https://www.spinics.net/lists/linux-bluetooth/msg74918.html

[RFC] Bluetooth: Add new serdev based driver for 3-Wire attached controllers
https://www.spinics.net/lists/linux-bluetooth/msg74839.html

Thanks,
Archie

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

* Re: [PATCH v2 1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag
  2021-07-26  4:02       ` Archie Pusaka
@ 2021-07-29 11:54         ` Marcel Holtmann
  0 siblings, 0 replies; 9+ messages in thread
From: Marcel Holtmann @ 2021-07-29 11:54 UTC (permalink / raw)
  To: Archie Pusaka
  Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka,
	Abhishek Pandit-Subedi, Hilda Wu, Johan Hedberg,
	Luiz Augusto von Dentz, linux-kernel

Hi Archie,

>>>>> Some RTL chips resets the FW on suspend, so wakeup is disabled on
>>>>> those chips. This patch introduces this WAKEUP_DISABLE flag so that
>>>>> chips that doesn't reset FW on suspend can leave the flag unset and
>>>>> is allowed to wake the host.
>>>>> 
>>>>> This patch also left RTL8822 WAKEUP_DISABLE flag unset, therefore
>>>>> allowing it to wake the host, and preventing reprobing on resume.
>>>>> 
>>>>> Signed-off-by: Archie Pusaka <apusaka@chromium.org>
>>>>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>>>>> Reviewed-by: Hilda Wu <hildawu@realtek.com>
>>>>> 
>>>>> ---
>>>>> 
>>>>> Changes in v2:
>>>>> * Remove unnecessary variable
>>>>> 
>>>>> drivers/bluetooth/hci_h5.c | 83 +++++++++++++++++++++++++++-----------
>>>>> 1 file changed, 59 insertions(+), 24 deletions(-)
>>>> 
>>>> so the set does not apply cleanly to bluetooth-next
>>>> 
>>>> Applying: Bluetooth: hci_h5: Add runtime suspend
>>>> error: patch failed: drivers/bluetooth/hci_h5.c:11
>>>> error: drivers/bluetooth/hci_h5.c: patch does not apply
>>> 
>>> Hmm, it applies cleanly for me. Not sure what's going on.
>>> Anyway I rebased and made a little change as v3, please take a look!
>> 
>> the v3 applied cleanly.
>> 
>>>> 
>>>> 
>>>> And I am really close to not accepting any patches for hci_h5.c anymore. This thing turns into crazy hacking and nobody is taking my hint to redo this as clean H:5 3-Wire serdev standalone driver.
>>> 
>>> Pardon my unfamiliarity, but could you share more about your vision of
>>> a clean h5 driver? Should the RTL component be moved out to btrtl?
>>> Do we have something as a reference?
>> 
>> so a while back I send a bt3wire.c sample driver around. That would be a good starting point.
>> 
>> Anyhow, the problem is that hci_uart.ko is inherent a line discipline driver from 2.4.x kernel days and it has been stacked and hacked on top of it. It has become a burden, especially in the light that you can have clean serdev based drivers now (like btmtkuart.c).
>> 
>> And yes, it would be following the 3-Wire H:5 spec and then deal with vendor specific details like btusb.c for example. And my hope would be that especially in the Realtek and Broadcom (RPi3 etc.) cases this can move into vendor specific blocks and shared between USB and UART transports.
>> 
>> I also send around a btuart.c sample driver that is solely serdev based and should replace all the cases where we have H:4 as transport.
>> 
> 
> Thanks for the pointers!
> 
> The files you mentioned are rather hard to find, so below I paste the
> URL where I found them in case anyone else is also interested.
> 
> [RFC v2] Bluetooth: Add new serdev based driver for UART attached controllers
> https://www.spinics.net/lists/linux-bluetooth/msg74918.html
> 
> [RFC] Bluetooth: Add new serdev based driver for 3-Wire attached controllers
> https://www.spinics.net/lists/linux-bluetooth/msg74839.html

exactly these. I posted my initial work so that it can be continued by people with easier access to hardware.

Regards

Marcel


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

end of thread, other threads:[~2021-07-29 11:54 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-15 14:51 [PATCH v2 1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag Archie Pusaka
2021-07-15 14:51 ` [PATCH v2 2/3] Bluetooth: hci_h5: btrtl: Maintain flow control if wakeup is enabled Archie Pusaka
2021-07-15 14:52 ` [PATCH v2 3/3] Bluetooth: hci_h5: Add runtime suspend Archie Pusaka
2021-07-15 16:15 ` [v2,1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag bluez.test.bot
2021-07-22 14:32 ` [PATCH v2 1/3] " Marcel Holtmann
2021-07-23 11:42   ` Archie Pusaka
2021-07-23 12:17     ` Marcel Holtmann
2021-07-26  4:02       ` Archie Pusaka
2021-07-29 11:54         ` 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).