[PATCH v2 0/4] Bluetooth: hci_bcm: Add wake-up and PM runtime support

[PATCH v2 0/4] Bluetooth: hci_bcm: Add wake-up and PM runtime support

[Frederic Danis]

Add wake-up capabilities by retrieveing interruption used by BCM device in ACPI
table.
Add PM runtime support.

v1->v2:
- Split 1st patch between general wake-up capability and T100TA IRQ fix
- Replace multiple "if ... else if" by switch in bcm_resource()
- Move code to limit number of #ifdef
- Use DMI info to restrict IRQ to T100TA
- Split 2nd patch to prepare PM runtime support in separated patch
- Tested with and without CONFIG_PM_SLEEP and CONFIG_PM.

Frederic Danis (4):
  Bluetooth: hci_bcm: Add wake-up capability
  Bluetooth: hci_bcm: Fix IRQ polarity for T100
  Bluetooth: hci_bcm: Prepare PM runtime support
  Bluetooth: hci_bcm: Add suspend/resume runtime PM functions

 drivers/bluetooth/hci_bcm.c | 348 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 312 insertions(+), 36 deletions(-)

-- 
1.9.1

[PATCH v2 1/4] Bluetooth: hci_bcm: Add wake-up capability

[Frederic Danis]

Retrieve the Interruption used by BCM device, which can be declared
as Interruption or GpioInt in the ACPI table.
Retrieve IRQ polarity from the ACPI table to use it for host_wake_active
parameter of Setup Sleep vendor specific command.
Configure BCM device to wake-up the host.
Enable IRQ wake while suspended.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_bcm.c | 160 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 150 insertions(+), 10 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index fbad52f..f306541 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -31,6 +31,7 @@
 #include <linux/clk.h>
 #include <linux/gpio/consumer.h>
 #include <linux/tty.h>
+#include <linux/interrupt.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -51,6 +52,8 @@ struct bcm_device {
 	bool			clk_enabled;
 
 	u32			init_speed;
+	int			irq;
+	u8			irq_polarity;
 
 #ifdef CONFIG_PM_SLEEP
 	struct hci_uart		*hu;
@@ -149,6 +152,86 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static irqreturn_t bcm_host_wake(int irq, void *data)
+{
+	struct bcm_device *bdev = data;
+
+	bt_dev_dbg(bdev, "Host wake IRQ");
+
+	return IRQ_HANDLED;
+}
+
+static int bcm_request_irq(struct bcm_data *bcm)
+{
+	struct bcm_device *bdev = bcm->dev;
+	int err = 0;
+
+	/* If this is not a platform device, do not enable PM functionalities */
+	mutex_lock(&bcm_device_lock);
+	if (!bcm_device_exists(bdev)) {
+		err = -ENODEV;
+		goto unlock;
+	}
+
+	if (bdev->irq > 0) {
+		err = devm_request_irq(&bdev->pdev->dev, bdev->irq,
+				       bcm_host_wake, IRQF_TRIGGER_RISING,
+				       "host_wake", bdev);
+		if (err)
+			goto unlock;
+
+		device_init_wakeup(&bdev->pdev->dev, true);
+	}
+
+unlock:
+	mutex_unlock(&bcm_device_lock);
+
+	return err;
+}
+
+static const struct bcm_set_sleep_mode default_sleep_params = {
+	.sleep_mode = 1,	/* 0=Disabled, 1=UART, 2=Reserved, 3=USB */
+	.idle_host = 2,		/* idle threshold HOST, in 300ms */
+	.idle_dev = 2,		/* idle threshold device, in 300ms */
+	.bt_wake_active = 1,	/* BT_WAKE active mode: 1 = high, 0 = low */
+	.host_wake_active = 0,	/* HOST_WAKE active mode: 1 = high, 0 = low */
+	.allow_host_sleep = 1,	/* Allow host sleep in SCO flag */
+	.combine_modes = 0,	/* Combine sleep and LPM flag */
+	.tristate_control = 0,	/* Allow tri-state control of UART tx flag */
+	/* Irrelevant USB flags */
+	.usb_auto_sleep = 0,
+	.usb_resume_timeout = 0,
+	.pulsed_host_wake = 0,
+	.break_to_host = 0
+};
+
+static int bcm_setup_sleep(struct hci_uart *hu)
+{
+	struct bcm_data *bcm = hu->priv;
+	struct sk_buff *skb;
+	struct bcm_set_sleep_mode sleep_params = default_sleep_params;
+
+	sleep_params.host_wake_active = !bcm->dev->irq_polarity;
+
+	skb = __hci_cmd_sync(hu->hdev, 0xfc27, sizeof(sleep_params),
+			     &sleep_params, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		int err = PTR_ERR(skb);
+		bt_dev_err(hu->hdev, "Sleep VSC failed (%d)", err);
+		return err;
+	}
+	kfree_skb(skb);
+
+	bt_dev_dbg(hu->hdev, "Set Sleep Parameters VSC succeeded");
+
+	return 0;
+}
+#else
+static inline int bcm_request_irq(struct bcm_data *bcm) { return 0; }
+static inline int bcm_setup_sleep(struct hci_uart *hu) { return 0; }
+#endif
+
 static int bcm_open(struct hci_uart *hu)
 {
 	struct bcm_data *bcm;
@@ -178,13 +261,11 @@ static int bcm_open(struct hci_uart *hu)
 #ifdef CONFIG_PM_SLEEP
 			dev->hu = hu;
 #endif
+			bcm_gpio_set_power(bcm->dev, true);
 			break;
 		}
 	}
 
-	if (bcm->dev)
-		bcm_gpio_set_power(bcm->dev, true);
-
 	mutex_unlock(&bcm_device_lock);
 
 	return 0;
@@ -193,15 +274,21 @@ static int bcm_open(struct hci_uart *hu)
 static int bcm_close(struct hci_uart *hu)
 {
 	struct bcm_data *bcm = hu->priv;
+	struct bcm_device *bdev = bcm->dev;
 
 	bt_dev_dbg(hu->hdev, "hu %p", hu);
 
 	/* Protect bcm->dev against removal of the device or driver */
 	mutex_lock(&bcm_device_lock);
-	if (bcm_device_exists(bcm->dev)) {
-		bcm_gpio_set_power(bcm->dev, false);
+	if (bcm_device_exists(bdev)) {
+		bcm_gpio_set_power(bdev, false);
 #ifdef CONFIG_PM_SLEEP
-		bcm->dev->hu = NULL;
+		if (device_can_wakeup(&bdev->pdev->dev)) {
+			devm_free_irq(&bdev->pdev->dev, bdev->irq, bdev);
+			device_init_wakeup(&bdev->pdev->dev, false);
+		}
+
+		bdev->hu = NULL;
 #endif
 	}
 	mutex_unlock(&bcm_device_lock);
@@ -227,6 +314,7 @@ static int bcm_flush(struct hci_uart *hu)
 
 static int bcm_setup(struct hci_uart *hu)
 {
+	struct bcm_data *bcm = hu->priv;
 	char fw_name[64];
 	const struct firmware *fw;
 	unsigned int speed;
@@ -281,6 +369,12 @@ finalize:
 	release_firmware(fw);
 
 	err = btbcm_finalize(hu->hdev);
+	if (err)
+		return err;
+
+	err = bcm_request_irq(bcm);
+	if (!err)
+		err = bcm_setup_sleep(hu);
 
 	return err;
 }
@@ -335,6 +429,7 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
 static int bcm_suspend(struct device *dev)
 {
 	struct bcm_device *bdev = platform_get_drvdata(to_platform_device(dev));
+	int error;
 
 	bt_dev_dbg(bdev, "suspend: is_suspended %d", bdev->is_suspended);
 
@@ -357,6 +452,12 @@ static int bcm_suspend(struct device *dev)
 		mdelay(15);
 	}
 
+	if (device_may_wakeup(&bdev->pdev->dev)) {
+		error = enable_irq_wake(bdev->irq);
+		if (!error)
+			bt_dev_dbg(bdev, "BCM irq: enabled");
+	}
+
 unlock:
 	mutex_unlock(&bcm_device_lock);
 
@@ -375,6 +476,11 @@ static int bcm_resume(struct device *dev)
 	if (!bdev->hu)
 		goto unlock;
 
+	if (device_may_wakeup(&bdev->pdev->dev)) {
+		disable_irq_wake(bdev->irq);
+		bt_dev_dbg(bdev, "BCM irq: disabled");
+	}
+
 	if (bdev->device_wakeup) {
 		gpiod_set_value(bdev->device_wakeup, true);
 		bt_dev_dbg(bdev, "resume, delaying 15 ms");
@@ -397,10 +503,12 @@ unlock:
 
 static const struct acpi_gpio_params device_wakeup_gpios = { 0, 0, false };
 static const struct acpi_gpio_params shutdown_gpios = { 1, 0, false };
+static const struct acpi_gpio_params host_wakeup_gpios = { 2, 0, false };
 
 static const struct acpi_gpio_mapping acpi_bcm_default_gpios[] = {
 	{ "device-wakeup-gpios", &device_wakeup_gpios, 1 },
 	{ "shutdown-gpios", &shutdown_gpios, 1 },
+	{ "host-wakeup-gpios", &host_wakeup_gpios, 1 },
 	{ },
 };
 
@@ -408,13 +516,30 @@ static const struct acpi_gpio_mapping acpi_bcm_default_gpios[] = {
 static int bcm_resource(struct acpi_resource *ares, void *data)
 {
 	struct bcm_device *dev = data;
-
-	if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
-		struct acpi_resource_uart_serialbus *sb;
-
+	struct acpi_resource_extended_irq *irq;
+	struct acpi_resource_gpio *gpio;
+	struct acpi_resource_uart_serialbus *sb;
+
+	switch (ares->type) {
+	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
+		irq = &ares->data.extended_irq;
+		dev->irq_polarity = irq->polarity;
+		break;
+
+	case ACPI_RESOURCE_TYPE_GPIO:
+		gpio = &ares->data.gpio;
+		if (gpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT)
+			dev->irq_polarity = gpio->polarity;
+		break;
+
+	case ACPI_RESOURCE_TYPE_SERIAL_BUS:
 		sb = &ares->data.uart_serial_bus;
 		if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_UART)
 			dev->init_speed = sb->default_baud_rate;
+		break;
+
+	default:
+		break;
 	}
 
 	/* Always tell the ACPI core to skip this resource */
@@ -453,6 +578,21 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 	if (IS_ERR(dev->shutdown))
 		return PTR_ERR(dev->shutdown);
 
+	/* IRQ can be declared in ACPI table as Interrupt or GpioInt */
+	dev->irq = platform_get_irq(pdev, 0);
+	if (dev->irq <= 0) {
+		struct gpio_desc *gpio;
+
+		gpio = devm_gpiod_get_optional(&pdev->dev, "host-wakeup",
+					       GPIOD_IN);
+		if (IS_ERR(gpio))
+			return PTR_ERR(gpio);
+
+		dev->irq = gpiod_to_irq(gpio);
+	}
+
+	dev_info(&pdev->dev, "BCM irq: %d\n", dev->irq);
+
 	/* Make sure at-least one of the GPIO is defined and that
 	 * a name is specified for this instance
 	 */
-- 
1.9.1

[PATCH v2 2/4] Bluetooth: hci_bcm: Fix IRQ polarity for T100

[Frederic Danis]

ACPI table for BCM2E39 of T100TA is not correct.
Invert irq_polarity for this device.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_bcm.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index f306541..efb9566 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -32,6 +32,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/tty.h>
 #include <linux/interrupt.h>
+#include <linux/dmi.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -546,6 +547,20 @@ static int bcm_resource(struct acpi_resource *ares, void *data)
 	return 1;
 }
 
+/* IRQ polarity of some chipset are not defined correctly in ACPI table. */
+static const struct dmi_system_id bcm_wrong_irq_dmi_table[] = {
+#if defined(CONFIG_DMI) && defined(CONFIG_X86)
+	{
+		/* Asus T100TA */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "T100TA"),
+		},
+	},
+#endif
+	{ }
+};
+
 static int bcm_acpi_probe(struct bcm_device *dev)
 {
 	struct platform_device *pdev = dev->pdev;
@@ -608,6 +623,12 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 
 	acpi_dev_get_resources(adev, &resources, bcm_resource, dev);
 
+	if (strstr(id->id, "BCM2E39") &&
+	    dmi_check_system(bcm_wrong_irq_dmi_table)) {
+		bt_dev_dbg(dev, "Fix irq polarity");
+		dev->irq_polarity = !dev->irq_polarity;
+	}
+
 	return 0;
 }
 #else
-- 
1.9.1

[PATCH v2 3/4] Bluetooth: hci_bcm: Prepare PM runtime support

[Frederic Danis]

Change some CONFIG_PM_SLEEP to CONFIG_PM as hu and is_suspended parameters
will be used during PM runtime callbacks.

Add __bcm_suspend() and __bcm_resume() which performs link management for
PM callbacks.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_bcm.c | 70 ++++++++++++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 29 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index efb9566..7f63f2b 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -56,7 +56,7 @@ struct bcm_device {
 	int			irq;
 	u8			irq_polarity;
 
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
 	struct hci_uart		*hu;
 	bool			is_suspended; /* suspend/resume flag */
 #endif
@@ -153,7 +153,7 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
 static irqreturn_t bcm_host_wake(int irq, void *data)
 {
 	struct bcm_device *bdev = data;
@@ -259,7 +259,7 @@ static int bcm_open(struct hci_uart *hu)
 		if (hu->tty->dev->parent == dev->pdev->dev.parent) {
 			bcm->dev = dev;
 			hu->init_speed = dev->init_speed;
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
 			dev->hu = hu;
 #endif
 			bcm_gpio_set_power(bcm->dev, true);
@@ -283,7 +283,7 @@ static int bcm_close(struct hci_uart *hu)
 	mutex_lock(&bcm_device_lock);
 	if (bcm_device_exists(bdev)) {
 		bcm_gpio_set_power(bdev, false);
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
 		if (device_can_wakeup(&bdev->pdev->dev)) {
 			devm_free_irq(&bdev->pdev->dev, bdev->irq, bdev);
 			device_init_wakeup(&bdev->pdev->dev, false);
@@ -425,6 +425,41 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
 	return skb_dequeue(&bcm->txq);
 }
 
+#ifdef CONFIG_PM
+static void __bcm_suspend(struct bcm_device *bdev)
+{
+	if (!bdev->is_suspended && bdev->hu) {
+		hci_uart_set_flow_control(bdev->hu, true);
+
+		/* Once this returns, driver suspends BT via GPIO */
+		bdev->is_suspended = true;
+	}
+
+	/* Suspend the device */
+	if (bdev->device_wakeup) {
+		gpiod_set_value(bdev->device_wakeup, false);
+		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
+		mdelay(15);
+	}
+}
+
+static void __bcm_resume(struct bcm_device *bdev)
+{
+	if (bdev->device_wakeup) {
+		gpiod_set_value(bdev->device_wakeup, true);
+		bt_dev_dbg(bdev, "resume, delaying 15 ms");
+		mdelay(15);
+	}
+
+	/* When this executes, the device has woken up already */
+	if (bdev->is_suspended && bdev->hu) {
+		bdev->is_suspended = false;
+
+		hci_uart_set_flow_control(bdev->hu, false);
+	}
+}
+#endif
+
 #ifdef CONFIG_PM_SLEEP
 /* Platform suspend callback */
 static int bcm_suspend(struct device *dev)
@@ -439,19 +474,7 @@ static int bcm_suspend(struct device *dev)
 	if (!bdev->hu)
 		goto unlock;
 
-	if (!bdev->is_suspended) {
-		hci_uart_set_flow_control(bdev->hu, true);
-
-		/* Once this callback returns, driver suspends BT via GPIO */
-		bdev->is_suspended = true;
-	}
-
-	/* Suspend the device */
-	if (bdev->device_wakeup) {
-		gpiod_set_value(bdev->device_wakeup, false);
-		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
-		mdelay(15);
-	}
+	__bcm_suspend(bdev);
 
 	if (device_may_wakeup(&bdev->pdev->dev)) {
 		error = enable_irq_wake(bdev->irq);
@@ -482,18 +505,7 @@ static int bcm_resume(struct device *dev)
 		bt_dev_dbg(bdev, "BCM irq: disabled");
 	}
 
-	if (bdev->device_wakeup) {
-		gpiod_set_value(bdev->device_wakeup, true);
-		bt_dev_dbg(bdev, "resume, delaying 15 ms");
-		mdelay(15);
-	}
-
-	/* When this callback executes, the device has woken up already */
-	if (bdev->is_suspended) {
-		bdev->is_suspended = false;
-
-		hci_uart_set_flow_control(bdev->hu, false);
-	}
+	__bcm_resume(bdev);
 
 unlock:
 	mutex_unlock(&bcm_device_lock);
-- 
1.9.1

[PATCH v2 4/4] Bluetooth: hci_bcm: Add suspend/resume runtime PM functions

[Frederic Danis]

Adds autosuspend runtime functionality to BCM UART driver.
Autosuspend is enabled at end of bcm_setup.

Add a work queue to be able to perform pm_runtime commands during bcm_recv
which is called by hci_uart_tty_receive() under spinlock.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_bcm.c | 111 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 107 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 7f63f2b..a226249 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -33,6 +33,7 @@
 #include <linux/tty.h>
 #include <linux/interrupt.h>
 #include <linux/dmi.h>
+#include <linux/pm_runtime.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -40,6 +41,8 @@
 #include "btbcm.h"
 #include "hci_uart.h"
 
+#define BCM_AUTOSUSPEND_DELAY	5000 /* default autosleep delay */
+
 struct bcm_device {
 	struct list_head	list;
 
@@ -67,6 +70,9 @@ struct bcm_data {
 	struct sk_buff_head	txq;
 
 	struct bcm_device	*dev;
+#ifdef CONFIG_PM
+	struct work_struct	pm_work;
+#endif
 };
 
 /* List of BCM BT UART devices */
@@ -160,6 +166,10 @@ static irqreturn_t bcm_host_wake(int irq, void *data)
 
 	bt_dev_dbg(bdev, "Host wake IRQ");
 
+	pm_runtime_get(&bdev->pdev->dev);
+	pm_runtime_mark_last_busy(&bdev->pdev->dev);
+	pm_runtime_put_autosuspend(&bdev->pdev->dev);
+
 	return IRQ_HANDLED;
 }
 
@@ -183,6 +193,12 @@ static int bcm_request_irq(struct bcm_data *bcm)
 			goto unlock;
 
 		device_init_wakeup(&bdev->pdev->dev, true);
+
+		pm_runtime_set_autosuspend_delay(&bdev->pdev->dev,
+						 BCM_AUTOSUSPEND_DELAY);
+		pm_runtime_use_autosuspend(&bdev->pdev->dev);
+		pm_runtime_set_active(&bdev->pdev->dev);
+		pm_runtime_enable(&bdev->pdev->dev);
 	}
 
 unlock:
@@ -198,7 +214,7 @@ static const struct bcm_set_sleep_mode default_sleep_params = {
 	.bt_wake_active = 1,	/* BT_WAKE active mode: 1 = high, 0 = low */
 	.host_wake_active = 0,	/* HOST_WAKE active mode: 1 = high, 0 = low */
 	.allow_host_sleep = 1,	/* Allow host sleep in SCO flag */
-	.combine_modes = 0,	/* Combine sleep and LPM flag */
+	.combine_modes = 1,	/* Combine sleep and LPM flag */
 	.tristate_control = 0,	/* Allow tri-state control of UART tx flag */
 	/* Irrelevant USB flags */
 	.usb_auto_sleep = 0,
@@ -228,9 +244,28 @@ static int bcm_setup_sleep(struct hci_uart *hu)
 
 	return 0;
 }
+
+static void bcm_pm_runtime_work(struct work_struct *work)
+{
+	struct bcm_data *bcm = container_of(work, struct bcm_data, pm_work);
+
+	mutex_lock(&bcm_device_lock);
+	if (bcm_device_exists(bcm->dev)) {
+		pm_runtime_get(&bcm->dev->pdev->dev);
+		pm_runtime_mark_last_busy(&bcm->dev->pdev->dev);
+		pm_runtime_put_autosuspend(&bcm->dev->pdev->dev);
+	}
+	mutex_unlock(&bcm_device_lock);
+}
+
+static bool bcm_schedule_work(struct bcm_data *bcm)
+{
+	return schedule_work(&bcm->pm_work);
+}
 #else
 static inline int bcm_request_irq(struct bcm_data *bcm) { return 0; }
 static inline int bcm_setup_sleep(struct hci_uart *hu) { return 0; }
+static inline bool bcm_schedule_work(struct bcm_data *bcm) { return false; }
 #endif
 
 static int bcm_open(struct hci_uart *hu)
@@ -261,6 +296,7 @@ static int bcm_open(struct hci_uart *hu)
 			hu->init_speed = dev->init_speed;
 #ifdef CONFIG_PM
 			dev->hu = hu;
+			INIT_WORK(&bcm->pm_work, bcm_pm_runtime_work);
 #endif
 			bcm_gpio_set_power(bcm->dev, true);
 			break;
@@ -284,6 +320,11 @@ static int bcm_close(struct hci_uart *hu)
 	if (bcm_device_exists(bdev)) {
 		bcm_gpio_set_power(bdev, false);
 #ifdef CONFIG_PM
+		cancel_work_sync(&bcm->pm_work);
+
+		pm_runtime_disable(&bdev->pdev->dev);
+		pm_runtime_set_suspended(&bdev->pdev->dev);
+
 		if (device_can_wakeup(&bdev->pdev->dev)) {
 			devm_free_irq(&bdev->pdev->dev, bdev->irq, bdev);
 			device_init_wakeup(&bdev->pdev->dev, false);
@@ -393,6 +434,13 @@ static int bcm_recv(struct hci_uart *hu, const void *data, int count)
 	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
 		return -EUNATCH;
 
+	/* mutex_lock/unlock can not be used here as this function is called
+	 * from hci_uart_tty_receive under spinlock.
+	 * Defer pm_runtime_* calls to work queue
+	 */
+	if (bcm->dev)
+		bcm_schedule_work(bcm);
+
 	bcm->rx_skb = h4_recv_buf(hu->hdev, bcm->rx_skb, data, count,
 				  bcm_recv_pkts, ARRAY_SIZE(bcm_recv_pkts));
 	if (IS_ERR(bcm->rx_skb)) {
@@ -421,8 +469,27 @@ static int bcm_enqueue(struct hci_uart *hu, struct sk_buff *skb)
 static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
 {
 	struct bcm_data *bcm = hu->priv;
+	struct sk_buff *skb = NULL;
+	struct bcm_device *bdev = NULL;
+
+	mutex_lock(&bcm_device_lock);
+
+	if (bcm_device_exists(bcm->dev)) {
+		bdev = bcm->dev;
+		pm_runtime_get_sync(&bdev->pdev->dev);
+		/* Shall be resumed here */
+	}
+
+	skb = skb_dequeue(&bcm->txq);
+
+	if (bdev) {
+		pm_runtime_mark_last_busy(&bdev->pdev->dev);
+		pm_runtime_put_autosuspend(&bdev->pdev->dev);
+	}
 
-	return skb_dequeue(&bcm->txq);
+	mutex_unlock(&bcm_device_lock);
+
+	return skb;
 }
 
 #ifdef CONFIG_PM
@@ -458,6 +525,34 @@ static void __bcm_resume(struct bcm_device *bdev)
 		hci_uart_set_flow_control(bdev->hu, false);
 	}
 }
+
+static int bcm_runtime_suspend(struct device *dev)
+{
+	struct bcm_device *bdev = platform_get_drvdata(to_platform_device(dev));
+
+	bt_dev_dbg(bdev, "");
+
+	/* bcm_device_lock held is not required here as bcm_runtime_suspend
+	 * is only called when device is attached.
+	 */
+	__bcm_suspend(bdev);
+
+	return 0;
+}
+
+static int bcm_runtime_resume(struct device *dev)
+{
+	struct bcm_device *bdev = platform_get_drvdata(to_platform_device(dev));
+
+	bt_dev_dbg(bdev, "");
+
+	/* bcm_device_lock held is not required here as bcm_runtime_resume
+	 * is only called when device is attached.
+	 */
+	__bcm_resume(bdev);
+
+	return 0;
+}
 #endif
 
 #ifdef CONFIG_PM_SLEEP
@@ -474,7 +569,8 @@ static int bcm_suspend(struct device *dev)
 	if (!bdev->hu)
 		goto unlock;
 
-	__bcm_suspend(bdev);
+	if (pm_runtime_active(dev))
+		__bcm_suspend(bdev);
 
 	if (device_may_wakeup(&bdev->pdev->dev)) {
 		error = enable_irq_wake(bdev->irq);
@@ -510,6 +606,10 @@ static int bcm_resume(struct device *dev)
 unlock:
 	mutex_unlock(&bcm_device_lock);
 
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	return 0;
 }
 #endif
@@ -726,7 +826,10 @@ MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
 #endif
 
 /* Platform suspend and resume callbacks */
-static SIMPLE_DEV_PM_OPS(bcm_pm_ops, bcm_suspend, bcm_resume);
+static const struct dev_pm_ops bcm_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(bcm_suspend, bcm_resume)
+	SET_RUNTIME_PM_OPS(bcm_runtime_suspend, bcm_runtime_resume, NULL)
+};
 
 static struct platform_driver bcm_driver = {
 	.probe = bcm_probe,
-- 
1.9.1

Re: [PATCH v2 2/4] Bluetooth: hci_bcm: Fix IRQ polarity for T100

[Loic Poulain]

Hi Fred,

> +/* IRQ polarity of some chipset are not defined correctly in ACPI table. */
> +static const struct dmi_system_id bcm_wrong_irq_dmi_table[] = {
> +#if defined(CONFIG_DMI) && defined(CONFIG_X86)
> +	{
> +		/* Asus T100TA */
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "T100TA"),
> +		},
> +	},
> +#endif
> +	{ }
> +};
> +
>   static int bcm_acpi_probe(struct bcm_device *dev)
>   {
>   	struct platform_device *pdev = dev->pdev;
> @@ -608,6 +623,12 @@ static int bcm_acpi_probe(struct bcm_device *dev)
>
>   	acpi_dev_get_resources(adev, &resources, bcm_resource, dev);
>
> +	if (strstr(id->id, "BCM2E39") &&
> +	    dmi_check_system(bcm_wrong_irq_dmi_table)) {
> +		bt_dev_dbg(dev, "Fix irq polarity");
> +		dev->irq_polarity = !dev->irq_polarity;
> +	}

Since you now the right polarity on ASUS T100 you should just set the 
right one, not invert the current one. Imagine that T100 receive a BIOS
update with the polarity fixed, it will be inverted to a incorrect value.

Regards,
Loic

-- 
Intel Open Source Technology Center
http://oss.intel.com/

Re: [PATCH v2 3/4] Bluetooth: hci_bcm: Prepare PM runtime support

[Marcel Holtmann]

Hi Fred,

> Change some CONFIG_PM_SLEEP to CONFIG_PM as hu and is_suspended parameters
> will be used during PM runtime callbacks.
> 
> Add __bcm_suspend() and __bcm_resume() which performs link management for
> PM callbacks.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/hci_bcm.c | 70 ++++++++++++++++++++++++++-------------------
> 1 file changed, 41 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index efb9566..7f63f2b 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -56,7 +56,7 @@ struct bcm_device {
> 	int			irq;
> 	u8			irq_polarity;
> 
> -#ifdef CONFIG_PM_SLEEP
> +#ifdef CONFIG_PM
> 	struct hci_uart		*hu;
> 	bool			is_suspended; /* suspend/resume flag */
> #endif
> @@ -153,7 +153,7 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
> 	return 0;
> }
> 
> -#ifdef CONFIG_PM_SLEEP
> +#ifdef CONFIG_PM
> static irqreturn_t bcm_host_wake(int irq, void *data)
> {
> 	struct bcm_device *bdev = data;
> @@ -259,7 +259,7 @@ static int bcm_open(struct hci_uart *hu)
> 		if (hu->tty->dev->parent == dev->pdev->dev.parent) {
> 			bcm->dev = dev;
> 			hu->init_speed = dev->init_speed;
> -#ifdef CONFIG_PM_SLEEP
> +#ifdef CONFIG_PM
> 			dev->hu = hu;
> #endif
> 			bcm_gpio_set_power(bcm->dev, true);
> @@ -283,7 +283,7 @@ static int bcm_close(struct hci_uart *hu)
> 	mutex_lock(&bcm_device_lock);
> 	if (bcm_device_exists(bdev)) {
> 		bcm_gpio_set_power(bdev, false);
> -#ifdef CONFIG_PM_SLEEP
> +#ifdef CONFIG_PM
> 		if (device_can_wakeup(&bdev->pdev->dev)) {
> 			devm_free_irq(&bdev->pdev->dev, bdev->irq, bdev);
> 			device_init_wakeup(&bdev->pdev->dev, false);
> @@ -425,6 +425,41 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
> 	return skb_dequeue(&bcm->txq);
> }
> 
> +#ifdef CONFIG_PM
> +static void __bcm_suspend(struct bcm_device *bdev)
> +{
> +	if (!bdev->is_suspended && bdev->hu) {
> +		hci_uart_set_flow_control(bdev->hu, true);
> +
> +		/* Once this returns, driver suspends BT via GPIO */
> +		bdev->is_suspended = true;
> +	}
> +
> +	/* Suspend the device */
> +	if (bdev->device_wakeup) {
> +		gpiod_set_value(bdev->device_wakeup, false);
> +		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
> +		mdelay(15);
> +	}
> +}
> +
> +static void __bcm_resume(struct bcm_device *bdev)
> +{
> +	if (bdev->device_wakeup) {
> +		gpiod_set_value(bdev->device_wakeup, true);
> +		bt_dev_dbg(bdev, "resume, delaying 15 ms");
> +		mdelay(15);
> +	}
> +
> +	/* When this executes, the device has woken up already */
> +	if (bdev->is_suspended && bdev->hu) {
> +		bdev->is_suspended = false;
> +
> +		hci_uart_set_flow_control(bdev->hu, false);
> +	}
> +}
> +#endif
> +
> #ifdef CONFIG_PM_SLEEP
> /* Platform suspend callback */
> static int bcm_suspend(struct device *dev)
> @@ -439,19 +474,7 @@ static int bcm_suspend(struct device *dev)
> 	if (!bdev->hu)
> 		goto unlock;
> 
> -	if (!bdev->is_suspended) {
> -		hci_uart_set_flow_control(bdev->hu, true);
> -
> -		/* Once this callback returns, driver suspends BT via GPIO */
> -		bdev->is_suspended = true;
> -	}
> -
> -	/* Suspend the device */
> -	if (bdev->device_wakeup) {
> -		gpiod_set_value(bdev->device_wakeup, false);
> -		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
> -		mdelay(15);
> -	}
> +	__bcm_suspend(bdev);

I do not see a reason for this change and this will also cause compile time warnings since the code is now behind different config options.

You have to give me a bit of better description on what this change is actually for.

Regards

Marcel

Re: [PATCH v2 1/4] Bluetooth: hci_bcm: Add wake-up capability

[Marcel Holtmann]

Hi Fred,

> Retrieve the Interruption used by BCM device, which can be declared
> as Interruption or GpioInt in the ACPI table.
> Retrieve IRQ polarity from the ACPI table to use it for host_wake_active
> parameter of Setup Sleep vendor specific command.
> Configure BCM device to wake-up the host.
> Enable IRQ wake while suspended.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/hci_bcm.c | 160 +++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 150 insertions(+), 10 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH v2 2/4] Bluetooth: hci_bcm: Fix IRQ polarity for T100

[Marcel Holtmann]

Hi Fred,

> ACPI table for BCM2E39 of T100TA is not correct.
> Invert irq_polarity for this device.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/hci_bcm.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index f306541..efb9566 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -32,6 +32,7 @@
> #include <linux/gpio/consumer.h>
> #include <linux/tty.h>
> #include <linux/interrupt.h>
> +#include <linux/dmi.h>
> 
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -546,6 +547,20 @@ static int bcm_resource(struct acpi_resource *ares, void *data)
> 	return 1;
> }
> 
> +/* IRQ polarity of some chipset are not defined correctly in ACPI table. */
> +static const struct dmi_system_id bcm_wrong_irq_dmi_table[] = {
> +#if defined(CONFIG_DMI) && defined(CONFIG_X86)
> +	{
> +		/* Asus T100TA */

I think instead of a comment you could just fill in .ident here. Or is that suppose to be used for something else?

> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "T100TA"),

Why are we not using DMI_EXACT_MATCH here in the first place. We really know which one is broken, correct?

> +		},
> +	},
> +#endif

What is this #ifdef buying us? Is include/linux/dmi.h in some way that we can not have this defined all the time?

> +	{ }
> +};
> +
> static int bcm_acpi_probe(struct bcm_device *dev)
> {
> 	struct platform_device *pdev = dev->pdev;
> @@ -608,6 +623,12 @@ static int bcm_acpi_probe(struct bcm_device *dev)
> 
> 	acpi_dev_get_resources(adev, &resources, bcm_resource, dev);
> 
> +	if (strstr(id->id, "BCM2E39") &&

Lets not bother with that check and just always run through the DMI table here. Especially when using DMI_EXACT_MATCH that should not be a problem.

If however that causes a problem, then I prefer we actually set .driver_data in the ACPI module table and base the check on the .driver_data instead of checking the string here once more.

I really don't know if BCM2E39 is specific to a single design or manufacture or platform. I am not sure how good we are in not accidentally re-using these IDs.

> +	    dmi_check_system(bcm_wrong_irq_dmi_table)) {
> +		bt_dev_dbg(dev, "Fix irq polarity");
> +		dev->irq_polarity = !dev->irq_polarity;
> +	}
> +
> 	return 0;
> }
> #else

Regards

Marcel

Re: [PATCH v2 4/4] Bluetooth: hci_bcm: Add suspend/resume runtime PM functions

[Marcel Holtmann]

Hi Fred,

> Adds autosuspend runtime functionality to BCM UART driver.
> Autosuspend is enabled at end of bcm_setup.
> 
> Add a work queue to be able to perform pm_runtime commands during bcm_recv
> which is called by hci_uart_tty_receive() under spinlock.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/hci_bcm.c | 111 ++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 107 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 7f63f2b..a226249 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -33,6 +33,7 @@
> #include <linux/tty.h>
> #include <linux/interrupt.h>
> #include <linux/dmi.h>
> +#include <linux/pm_runtime.h>
> 
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -40,6 +41,8 @@
> #include "btbcm.h"
> #include "hci_uart.h"
> 
> +#define BCM_AUTOSUSPEND_DELAY	5000 /* default autosleep delay */
> +
> struct bcm_device {
> 	struct list_head	list;
> 
> @@ -67,6 +70,9 @@ struct bcm_data {
> 	struct sk_buff_head	txq;
> 
> 	struct bcm_device	*dev;
> +#ifdef CONFIG_PM
> +	struct work_struct	pm_work;
> +#endif
> };
> 
> /* List of BCM BT UART devices */
> @@ -160,6 +166,10 @@ static irqreturn_t bcm_host_wake(int irq, void *data)
> 
> 	bt_dev_dbg(bdev, "Host wake IRQ");
> 
> +	pm_runtime_get(&bdev->pdev->dev);
> +	pm_runtime_mark_last_busy(&bdev->pdev->dev);
> +	pm_runtime_put_autosuspend(&bdev->pdev->dev);
> +
> 	return IRQ_HANDLED;
> }
> 
> @@ -183,6 +193,12 @@ static int bcm_request_irq(struct bcm_data *bcm)
> 			goto unlock;
> 
> 		device_init_wakeup(&bdev->pdev->dev, true);
> +
> +		pm_runtime_set_autosuspend_delay(&bdev->pdev->dev,
> +						 BCM_AUTOSUSPEND_DELAY);
> +		pm_runtime_use_autosuspend(&bdev->pdev->dev);
> +		pm_runtime_set_active(&bdev->pdev->dev);
> +		pm_runtime_enable(&bdev->pdev->dev);
> 	}
> 
> unlock:
> @@ -198,7 +214,7 @@ static const struct bcm_set_sleep_mode default_sleep_params = {
> 	.bt_wake_active = 1,	/* BT_WAKE active mode: 1 = high, 0 = low */
> 	.host_wake_active = 0,	/* HOST_WAKE active mode: 1 = high, 0 = low */
> 	.allow_host_sleep = 1,	/* Allow host sleep in SCO flag */
> -	.combine_modes = 0,	/* Combine sleep and LPM flag */
> +	.combine_modes = 1,	/* Combine sleep and LPM flag */
> 	.tristate_control = 0,	/* Allow tri-state control of UART tx flag */
> 	/* Irrelevant USB flags */
> 	.usb_auto_sleep = 0,
> @@ -228,9 +244,28 @@ static int bcm_setup_sleep(struct hci_uart *hu)
> 
> 	return 0;
> }
> +
> +static void bcm_pm_runtime_work(struct work_struct *work)
> +{
> +	struct bcm_data *bcm = container_of(work, struct bcm_data, pm_work);
> +
> +	mutex_lock(&bcm_device_lock);
> +	if (bcm_device_exists(bcm->dev)) {
> +		pm_runtime_get(&bcm->dev->pdev->dev);
> +		pm_runtime_mark_last_busy(&bcm->dev->pdev->dev);
> +		pm_runtime_put_autosuspend(&bcm->dev->pdev->dev);
> +	}
> +	mutex_unlock(&bcm_device_lock);
> +}
> +
> +static bool bcm_schedule_work(struct bcm_data *bcm)
> +{
> +	return schedule_work(&bcm->pm_work);
> +}
> #else
> static inline int bcm_request_irq(struct bcm_data *bcm) { return 0; }
> static inline int bcm_setup_sleep(struct hci_uart *hu) { return 0; }
> +static inline bool bcm_schedule_work(struct bcm_data *bcm) { return false; }
> #endif
> 
> static int bcm_open(struct hci_uart *hu)
> @@ -261,6 +296,7 @@ static int bcm_open(struct hci_uart *hu)
> 			hu->init_speed = dev->init_speed;
> #ifdef CONFIG_PM
> 			dev->hu = hu;
> +			INIT_WORK(&bcm->pm_work, bcm_pm_runtime_work);
> #endif
> 			bcm_gpio_set_power(bcm->dev, true);
> 			break;
> @@ -284,6 +320,11 @@ static int bcm_close(struct hci_uart *hu)
> 	if (bcm_device_exists(bdev)) {
> 		bcm_gpio_set_power(bdev, false);
> #ifdef CONFIG_PM
> +		cancel_work_sync(&bcm->pm_work);
> +
> +		pm_runtime_disable(&bdev->pdev->dev);
> +		pm_runtime_set_suspended(&bdev->pdev->dev);
> +
> 		if (device_can_wakeup(&bdev->pdev->dev)) {
> 			devm_free_irq(&bdev->pdev->dev, bdev->irq, bdev);
> 			device_init_wakeup(&bdev->pdev->dev, false);
> @@ -393,6 +434,13 @@ static int bcm_recv(struct hci_uart *hu, const void *data, int count)
> 	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
> 		return -EUNATCH;
> 
> +	/* mutex_lock/unlock can not be used here as this function is called
> +	 * from hci_uart_tty_receive under spinlock.
> +	 * Defer pm_runtime_* calls to work queue
> +	 */
> +	if (bcm->dev)
> +		bcm_schedule_work(bcm);
> +
> 	bcm->rx_skb = h4_recv_buf(hu->hdev, bcm->rx_skb, data, count,
> 				  bcm_recv_pkts, ARRAY_SIZE(bcm_recv_pkts));
> 	if (IS_ERR(bcm->rx_skb)) {
> @@ -421,8 +469,27 @@ static int bcm_enqueue(struct hci_uart *hu, struct sk_buff *skb)
> static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
> {
> 	struct bcm_data *bcm = hu->priv;
> +	struct sk_buff *skb = NULL;
> +	struct bcm_device *bdev = NULL;
> +
> +	mutex_lock(&bcm_device_lock);
> +
> +	if (bcm_device_exists(bcm->dev)) {
> +		bdev = bcm->dev;
> +		pm_runtime_get_sync(&bdev->pdev->dev);
> +		/* Shall be resumed here */
> +	}
> +
> +	skb = skb_dequeue(&bcm->txq);
> +
> +	if (bdev) {
> +		pm_runtime_mark_last_busy(&bdev->pdev->dev);
> +		pm_runtime_put_autosuspend(&bdev->pdev->dev);
> +	}
> 
> -	return skb_dequeue(&bcm->txq);
> +	mutex_unlock(&bcm_device_lock);
> +
> +	return skb;
> }
> 
> #ifdef CONFIG_PM
> @@ -458,6 +525,34 @@ static void __bcm_resume(struct bcm_device *bdev)
> 		hci_uart_set_flow_control(bdev->hu, false);
> 	}
> }
> +
> +static int bcm_runtime_suspend(struct device *dev)
> +{
> +	struct bcm_device *bdev = platform_get_drvdata(to_platform_device(dev));
> +
> +	bt_dev_dbg(bdev, "");
> +
> +	/* bcm_device_lock held is not required here as bcm_runtime_suspend
> +	 * is only called when device is attached.
> +	 */
> +	__bcm_suspend(bdev);
> +
> +	return 0;
> +}
> +
> +static int bcm_runtime_resume(struct device *dev)
> +{
> +	struct bcm_device *bdev = platform_get_drvdata(to_platform_device(dev));
> +
> +	bt_dev_dbg(bdev, "");
> +
> +	/* bcm_device_lock held is not required here as bcm_runtime_resume
> +	 * is only called when device is attached.
> +	 */
> +	__bcm_resume(bdev);
> +
> +	return 0;
> +}
> #endif
> 
> #ifdef CONFIG_PM_SLEEP
> @@ -474,7 +569,8 @@ static int bcm_suspend(struct device *dev)
> 	if (!bdev->hu)
> 		goto unlock;
> 
> -	__bcm_suspend(bdev);
> +	if (pm_runtime_active(dev))
> +		__bcm_suspend(bdev);
> 
> 	if (device_may_wakeup(&bdev->pdev->dev)) {
> 		error = enable_irq_wake(bdev->irq);
> @@ -510,6 +606,10 @@ static int bcm_resume(struct device *dev)
> unlock:
> 	mutex_unlock(&bcm_device_lock);
> 
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
> 	return 0;
> }
> #endif
> @@ -726,7 +826,10 @@ MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
> #endif
> 
> /* Platform suspend and resume callbacks */
> -static SIMPLE_DEV_PM_OPS(bcm_pm_ops, bcm_suspend, bcm_resume);
> +static const struct dev_pm_ops bcm_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(bcm_suspend, bcm_resume)
> +	SET_RUNTIME_PM_OPS(bcm_runtime_suspend, bcm_runtime_resume, NULL)

I think you really need to explain why runtime suspend and system sleep is actually different for Broadcom based devices. Since for the Intel ones it turned out to be the same (at least for now).

> +};
> 
> static struct platform_driver bcm_driver = {
> 	.probe = bcm_probe,

Regards

Marcel

Re: [PATCH v2 3/4] Bluetooth: hci_bcm: Prepare PM runtime support

[Frederic Danis]

Hello Marcel,

On 04/09/2015 20:51, Marcel Holtmann wrote:
> Hi Fred,
>
>> Change some CONFIG_PM_SLEEP to CONFIG_PM as hu and is_suspended parameters
>> will be used during PM runtime callbacks.
>>
>> Add __bcm_suspend() and __bcm_resume() which performs link management for
>> PM callbacks.
>>
>> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
>> ---
>> drivers/bluetooth/hci_bcm.c | 70 ++++++++++++++++++++++++++-------------------
>> 1 file changed, 41 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> index efb9566..7f63f2b 100644
>> --- a/drivers/bluetooth/hci_bcm.c
>> +++ b/drivers/bluetooth/hci_bcm.c
>> @@ -56,7 +56,7 @@ struct bcm_device {
>> 	int			irq;
>> 	u8			irq_polarity;
>>
>> -#ifdef CONFIG_PM_SLEEP
>> +#ifdef CONFIG_PM
>> 	struct hci_uart		*hu;
>> 	bool			is_suspended; /* suspend/resume flag */
>> #endif
>> @@ -153,7 +153,7 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
>> 	return 0;
>> }
>>
>> -#ifdef CONFIG_PM_SLEEP
>> +#ifdef CONFIG_PM
>> static irqreturn_t bcm_host_wake(int irq, void *data)
>> {
>> 	struct bcm_device *bdev = data;
>> @@ -259,7 +259,7 @@ static int bcm_open(struct hci_uart *hu)
>> 		if (hu->tty->dev->parent == dev->pdev->dev.parent) {
>> 			bcm->dev = dev;
>> 			hu->init_speed = dev->init_speed;
>> -#ifdef CONFIG_PM_SLEEP
>> +#ifdef CONFIG_PM
>> 			dev->hu = hu;
>> #endif
>> 			bcm_gpio_set_power(bcm->dev, true);
>> @@ -283,7 +283,7 @@ static int bcm_close(struct hci_uart *hu)
>> 	mutex_lock(&bcm_device_lock);
>> 	if (bcm_device_exists(bdev)) {
>> 		bcm_gpio_set_power(bdev, false);
>> -#ifdef CONFIG_PM_SLEEP
>> +#ifdef CONFIG_PM
>> 		if (device_can_wakeup(&bdev->pdev->dev)) {
>> 			devm_free_irq(&bdev->pdev->dev, bdev->irq, bdev);
>> 			device_init_wakeup(&bdev->pdev->dev, false);
>> @@ -425,6 +425,41 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
>> 	return skb_dequeue(&bcm->txq);
>> }
>>
>> +#ifdef CONFIG_PM
>> +static void __bcm_suspend(struct bcm_device *bdev)
>> +{
>> +	if (!bdev->is_suspended && bdev->hu) {
>> +		hci_uart_set_flow_control(bdev->hu, true);
>> +
>> +		/* Once this returns, driver suspends BT via GPIO */
>> +		bdev->is_suspended = true;
>> +	}
>> +
>> +	/* Suspend the device */
>> +	if (bdev->device_wakeup) {
>> +		gpiod_set_value(bdev->device_wakeup, false);
>> +		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
>> +		mdelay(15);
>> +	}
>> +}
>> +
>> +static void __bcm_resume(struct bcm_device *bdev)
>> +{
>> +	if (bdev->device_wakeup) {
>> +		gpiod_set_value(bdev->device_wakeup, true);
>> +		bt_dev_dbg(bdev, "resume, delaying 15 ms");
>> +		mdelay(15);
>> +	}
>> +
>> +	/* When this executes, the device has woken up already */
>> +	if (bdev->is_suspended && bdev->hu) {
>> +		bdev->is_suspended = false;
>> +
>> +		hci_uart_set_flow_control(bdev->hu, false);
>> +	}
>> +}
>> +#endif
>> +
>> #ifdef CONFIG_PM_SLEEP
>> /* Platform suspend callback */
>> static int bcm_suspend(struct device *dev)
>> @@ -439,19 +474,7 @@ static int bcm_suspend(struct device *dev)
>> 	if (!bdev->hu)
>> 		goto unlock;
>>
>> -	if (!bdev->is_suspended) {
>> -		hci_uart_set_flow_control(bdev->hu, true);
>> -
>> -		/* Once this callback returns, driver suspends BT via GPIO */
>> -		bdev->is_suspended = true;
>> -	}
>> -
>> -	/* Suspend the device */
>> -	if (bdev->device_wakeup) {
>> -		gpiod_set_value(bdev->device_wakeup, false);
>> -		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
>> -		mdelay(15);
>> -	}
>> +	__bcm_suspend(bdev);
>
> I do not see a reason for this change and this will also cause compile time warnings since the code is now behind different config options.

I checked this and do not get any warning as CONFIG_PM_SLEEP selects 
CONFIG_PM.

> You have to give me a bit of better description on what this change is actually for.

This is the common part of runtime suspend and system sleep functions.
Please, see my reply to your comments on patch 4/4.

Regards

Fred

Re: [PATCH v2 4/4] Bluetooth: hci_bcm: Add suspend/resume runtime PM functions

[Frederic Danis]

Hello Marcel,

On 04/09/2015 21:15, Marcel Holtmann wrote:
> Hi Fred,
>
>> Adds autosuspend runtime functionality to BCM UART driver.
>> Autosuspend is enabled at end of bcm_setup.
>>
>> Add a work queue to be able to perform pm_runtime commands during bcm_recv
>> which is called by hci_uart_tty_receive() under spinlock.
>>
>> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
>> ---
>> drivers/bluetooth/hci_bcm.c | 111 ++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 107 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> index 7f63f2b..a226249 100644
>> --- a/drivers/bluetooth/hci_bcm.c
>> +++ b/drivers/bluetooth/hci_bcm.c
>> @@ -33,6 +33,7 @@
>> #include <linux/tty.h>
>> #include <linux/interrupt.h>
>> #include <linux/dmi.h>
>> +#include <linux/pm_runtime.h>
>>
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>> @@ -40,6 +41,8 @@
>> #include "btbcm.h"
>> #include "hci_uart.h"
>>
>> +#define BCM_AUTOSUSPEND_DELAY	5000 /* default autosleep delay */
>> +
>> struct bcm_device {
>> 	struct list_head	list;
>>
>> @@ -67,6 +70,9 @@ struct bcm_data {
>> 	struct sk_buff_head	txq;
>>
>> 	struct bcm_device	*dev;
>> +#ifdef CONFIG_PM
>> +	struct work_struct	pm_work;
>> +#endif
>> };
>>
>> /* List of BCM BT UART devices */
>> @@ -160,6 +166,10 @@ static irqreturn_t bcm_host_wake(int irq, void *data)
>>
>> 	bt_dev_dbg(bdev, "Host wake IRQ");
>>
>> +	pm_runtime_get(&bdev->pdev->dev);
>> +	pm_runtime_mark_last_busy(&bdev->pdev->dev);
>> +	pm_runtime_put_autosuspend(&bdev->pdev->dev);
>> +
>> 	return IRQ_HANDLED;
>> }
>>
>> @@ -183,6 +193,12 @@ static int bcm_request_irq(struct bcm_data *bcm)
>> 			goto unlock;
>>
>> 		device_init_wakeup(&bdev->pdev->dev, true);
>> +
>> +		pm_runtime_set_autosuspend_delay(&bdev->pdev->dev,
>> +						 BCM_AUTOSUSPEND_DELAY);
>> +		pm_runtime_use_autosuspend(&bdev->pdev->dev);
>> +		pm_runtime_set_active(&bdev->pdev->dev);
>> +		pm_runtime_enable(&bdev->pdev->dev);
>> 	}
>>
>> unlock:
>> @@ -198,7 +214,7 @@ static const struct bcm_set_sleep_mode default_sleep_params = {
>> 	.bt_wake_active = 1,	/* BT_WAKE active mode: 1 = high, 0 = low */
>> 	.host_wake_active = 0,	/* HOST_WAKE active mode: 1 = high, 0 = low */
>> 	.allow_host_sleep = 1,	/* Allow host sleep in SCO flag */
>> -	.combine_modes = 0,	/* Combine sleep and LPM flag */
>> +	.combine_modes = 1,	/* Combine sleep and LPM flag */
>> 	.tristate_control = 0,	/* Allow tri-state control of UART tx flag */
>> 	/* Irrelevant USB flags */
>> 	.usb_auto_sleep = 0,
>> @@ -228,9 +244,28 @@ static int bcm_setup_sleep(struct hci_uart *hu)
>>
>> 	return 0;
>> }
>> +
>> +static void bcm_pm_runtime_work(struct work_struct *work)
>> +{
>> +	struct bcm_data *bcm = container_of(work, struct bcm_data, pm_work);
>> +
>> +	mutex_lock(&bcm_device_lock);
>> +	if (bcm_device_exists(bcm->dev)) {
>> +		pm_runtime_get(&bcm->dev->pdev->dev);
>> +		pm_runtime_mark_last_busy(&bcm->dev->pdev->dev);
>> +		pm_runtime_put_autosuspend(&bcm->dev->pdev->dev);
>> +	}
>> +	mutex_unlock(&bcm_device_lock);
>> +}
>> +
>> +static bool bcm_schedule_work(struct bcm_data *bcm)
>> +{
>> +	return schedule_work(&bcm->pm_work);
>> +}
>> #else
>> static inline int bcm_request_irq(struct bcm_data *bcm) { return 0; }
>> static inline int bcm_setup_sleep(struct hci_uart *hu) { return 0; }
>> +static inline bool bcm_schedule_work(struct bcm_data *bcm) { return false; }
>> #endif
>>
>> static int bcm_open(struct hci_uart *hu)
>> @@ -261,6 +296,7 @@ static int bcm_open(struct hci_uart *hu)
>> 			hu->init_speed = dev->init_speed;
>> #ifdef CONFIG_PM
>> 			dev->hu = hu;
>> +			INIT_WORK(&bcm->pm_work, bcm_pm_runtime_work);
>> #endif
>> 			bcm_gpio_set_power(bcm->dev, true);
>> 			break;
>> @@ -284,6 +320,11 @@ static int bcm_close(struct hci_uart *hu)
>> 	if (bcm_device_exists(bdev)) {
>> 		bcm_gpio_set_power(bdev, false);
>> #ifdef CONFIG_PM
>> +		cancel_work_sync(&bcm->pm_work);
>> +
>> +		pm_runtime_disable(&bdev->pdev->dev);
>> +		pm_runtime_set_suspended(&bdev->pdev->dev);
>> +
>> 		if (device_can_wakeup(&bdev->pdev->dev)) {
>> 			devm_free_irq(&bdev->pdev->dev, bdev->irq, bdev);
>> 			device_init_wakeup(&bdev->pdev->dev, false);
>> @@ -393,6 +434,13 @@ static int bcm_recv(struct hci_uart *hu, const void *data, int count)
>> 	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
>> 		return -EUNATCH;
>>
>> +	/* mutex_lock/unlock can not be used here as this function is called
>> +	 * from hci_uart_tty_receive under spinlock.
>> +	 * Defer pm_runtime_* calls to work queue
>> +	 */
>> +	if (bcm->dev)
>> +		bcm_schedule_work(bcm);
>> +
>> 	bcm->rx_skb = h4_recv_buf(hu->hdev, bcm->rx_skb, data, count,
>> 				  bcm_recv_pkts, ARRAY_SIZE(bcm_recv_pkts));
>> 	if (IS_ERR(bcm->rx_skb)) {
>> @@ -421,8 +469,27 @@ static int bcm_enqueue(struct hci_uart *hu, struct sk_buff *skb)
>> static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
>> {
>> 	struct bcm_data *bcm = hu->priv;
>> +	struct sk_buff *skb = NULL;
>> +	struct bcm_device *bdev = NULL;
>> +
>> +	mutex_lock(&bcm_device_lock);
>> +
>> +	if (bcm_device_exists(bcm->dev)) {
>> +		bdev = bcm->dev;
>> +		pm_runtime_get_sync(&bdev->pdev->dev);
>> +		/* Shall be resumed here */
>> +	}
>> +
>> +	skb = skb_dequeue(&bcm->txq);
>> +
>> +	if (bdev) {
>> +		pm_runtime_mark_last_busy(&bdev->pdev->dev);
>> +		pm_runtime_put_autosuspend(&bdev->pdev->dev);
>> +	}
>>
>> -	return skb_dequeue(&bcm->txq);
>> +	mutex_unlock(&bcm_device_lock);
>> +
>> +	return skb;
>> }
>>
>> #ifdef CONFIG_PM
>> @@ -458,6 +525,34 @@ static void __bcm_resume(struct bcm_device *bdev)
>> 		hci_uart_set_flow_control(bdev->hu, false);
>> 	}
>> }
>> +
>> +static int bcm_runtime_suspend(struct device *dev)
>> +{
>> +	struct bcm_device *bdev = platform_get_drvdata(to_platform_device(dev));
>> +
>> +	bt_dev_dbg(bdev, "");
>> +
>> +	/* bcm_device_lock held is not required here as bcm_runtime_suspend
>> +	 * is only called when device is attached.
>> +	 */
>> +	__bcm_suspend(bdev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int bcm_runtime_resume(struct device *dev)
>> +{
>> +	struct bcm_device *bdev = platform_get_drvdata(to_platform_device(dev));
>> +
>> +	bt_dev_dbg(bdev, "");
>> +
>> +	/* bcm_device_lock held is not required here as bcm_runtime_resume
>> +	 * is only called when device is attached.
>> +	 */
>> +	__bcm_resume(bdev);
>> +
>> +	return 0;
>> +}
>> #endif
>>
>> #ifdef CONFIG_PM_SLEEP
>> @@ -474,7 +569,8 @@ static int bcm_suspend(struct device *dev)
>> 	if (!bdev->hu)
>> 		goto unlock;
>>
>> -	__bcm_suspend(bdev);
>> +	if (pm_runtime_active(dev))
>> +		__bcm_suspend(bdev);
>>
>> 	if (device_may_wakeup(&bdev->pdev->dev)) {
>> 		error = enable_irq_wake(bdev->irq);
>> @@ -510,6 +606,10 @@ static int bcm_resume(struct device *dev)
>> unlock:
>> 	mutex_unlock(&bcm_device_lock);
>>
>> +	pm_runtime_disable(dev);
>> +	pm_runtime_set_active(dev);
>> +	pm_runtime_enable(dev);
>> +
>> 	return 0;
>> }
>> #endif
>> @@ -726,7 +826,10 @@ MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
>> #endif
>>
>> /* Platform suspend and resume callbacks */
>> -static SIMPLE_DEV_PM_OPS(bcm_pm_ops, bcm_suspend, bcm_resume);
>> +static const struct dev_pm_ops bcm_pm_ops = {
>> +	SET_SYSTEM_SLEEP_PM_OPS(bcm_suspend, bcm_resume)
>> +	SET_RUNTIME_PM_OPS(bcm_runtime_suspend, bcm_runtime_resume, NULL)
>
> I think you really need to explain why runtime suspend and system sleep is actually different for Broadcom based devices. Since for the Intel ones it turned out to be the same (at least for now).

I dissociate system sleep and runtime suspend functions for the 
following reasons:

1) IRQ is enabled as a wake source only during system sleep, while this 
is not needed for runtime suspend.

2) runtime suspend functions do not need to protect usage of dev->hu as 
these functions are called with a valid dev->hu (these functions are 
disabled before dev->hu is set to NULL when BCM driver is closed).
System sleep functions need this protection as they can be called even 
when driver is not opened, and need to ensure that dev->hu is valid 
until GPIO and UART flow control management is performed.

Common part (GPIO and UART flow control management) for runtime suspend 
and system sleep moved to __bcm_suspend() and __bcm_resume() in previous 
patch.

Regards

Fred

Re: [PATCH v2 2/4] Bluetooth: hci_bcm: Fix IRQ polarity for T100

[Frederic Danis]

Hello Marcel,

On 04/09/2015 21:13, Marcel Holtmann wrote:
> Hi Fred,
>
>> ACPI table for BCM2E39 of T100TA is not correct.
>> Invert irq_polarity for this device.
>>
>> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
>> ---
>> drivers/bluetooth/hci_bcm.c | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> index f306541..efb9566 100644
>> --- a/drivers/bluetooth/hci_bcm.c
>> +++ b/drivers/bluetooth/hci_bcm.c
>> @@ -32,6 +32,7 @@
>> #include <linux/gpio/consumer.h>
>> #include <linux/tty.h>
>> #include <linux/interrupt.h>
>> +#include <linux/dmi.h>
>>
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>> @@ -546,6 +547,20 @@ static int bcm_resource(struct acpi_resource *ares, void *data)
>> 	return 1;
>> }
>>
>> +/* IRQ polarity of some chipset are not defined correctly in ACPI table. */
>> +static const struct dmi_system_id bcm_wrong_irq_dmi_table[] = {
>> +#if defined(CONFIG_DMI) && defined(CONFIG_X86)
>> +	{
>> +		/* Asus T100TA */
>
> I think instead of a comment you could just fill in .ident here. Or is that suppose to be used for something else?

OK

>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "T100TA"),
>
> Why are we not using DMI_EXACT_MATCH here in the first place. We really know which one is broken, correct?

Yes, I will use it.

>> +		},
>> +	},
>> +#endif
>
> What is this #ifdef buying us? Is include/linux/dmi.h in some way that we can not have this defined all the time?

OK, will be removed.

>> +	{ }
>> +};
>> +
>> static int bcm_acpi_probe(struct bcm_device *dev)
>> {
>> 	struct platform_device *pdev = dev->pdev;
>> @@ -608,6 +623,12 @@ static int bcm_acpi_probe(struct bcm_device *dev)
>>
>> 	acpi_dev_get_resources(adev, &resources, bcm_resource, dev);
>>
>> +	if (strstr(id->id, "BCM2E39") &&
>
> Lets not bother with that check and just always run through the DMI table here. Especially when using DMI_EXACT_MATCH that should not be a problem.

OK.
I will also use dmi_system_id.driver_data to store the correct polarity 
for T100TA.

> If however that causes a problem, then I prefer we actually set .driver_data in the ACPI module table and base the check on the .driver_data instead of checking the string here once more.
>
> I really don't know if BCM2E39 is specific to a single design or manufacture or platform. I am not sure how good we are in not accidentally re-using these IDs.
>
>> +	    dmi_check_system(bcm_wrong_irq_dmi_table)) {
>> +		bt_dev_dbg(dev, "Fix irq polarity");
>> +		dev->irq_polarity = !dev->irq_polarity;
>> +	}
>> +
>> 	return 0;
>> }
>> #else

Regards

Fred

RE: [PATCH v2 4/4] Bluetooth: hci_bcm: Add suspend/resume runtime PM functions

[Ilya Faenson]

Hi Fred,

-----Original Message-----
From: linux-bluetooth-owner@vger.kernel.org [mailto:linux-bluetooth-owner@v=
ger.kernel.org] On Behalf Of Frederic Danis
Sent: Monday, September 07, 2015 11:22 AM
To: Marcel Holtmann
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH v2 4/4] Bluetooth: hci_bcm: Add suspend/resume runtime =
PM functions

Hello Marcel,

On 04/09/2015 21:15, Marcel Holtmann wrote:
> Hi Fred,
>
>> Adds autosuspend runtime functionality to BCM UART driver.
>> Autosuspend is enabled at end of bcm_setup.
>>
>> Add a work queue to be able to perform pm_runtime commands during bcm_re=
cv
>> which is called by hci_uart_tty_receive() under spinlock.
>>
>> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
>> ---
>> drivers/bluetooth/hci_bcm.c | 111 ++++++++++++++++++++++++++++++++++++++=
++++--
>> 1 file changed, 107 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> index 7f63f2b..a226249 100644
>> --- a/drivers/bluetooth/hci_bcm.c
>> +++ b/drivers/bluetooth/hci_bcm.c
>> @@ -33,6 +33,7 @@
>> #include <linux/tty.h>
>> #include <linux/interrupt.h>
>> #include <linux/dmi.h>
>> +#include <linux/pm_runtime.h>
>>
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>> @@ -40,6 +41,8 @@
>> #include "btbcm.h"
>> #include "hci_uart.h"
>>
>> +#define BCM_AUTOSUSPEND_DELAY	5000 /* default autosleep delay */
>> +
>> struct bcm_device {
>> 	struct list_head	list;
>>
>> @@ -67,6 +70,9 @@ struct bcm_data {
>> 	struct sk_buff_head	txq;
>>
>> 	struct bcm_device	*dev;
>> +#ifdef CONFIG_PM
>> +	struct work_struct	pm_work;
>> +#endif
>> };
>>
>> /* List of BCM BT UART devices */
>> @@ -160,6 +166,10 @@ static irqreturn_t bcm_host_wake(int irq, void *dat=
a)
>>
>> 	bt_dev_dbg(bdev, "Host wake IRQ");
>>
>> +	pm_runtime_get(&bdev->pdev->dev);
>> +	pm_runtime_mark_last_busy(&bdev->pdev->dev);
>> +	pm_runtime_put_autosuspend(&bdev->pdev->dev);
>> +
>> 	return IRQ_HANDLED;
>> }
>>
>> @@ -183,6 +193,12 @@ static int bcm_request_irq(struct bcm_data *bcm)
>> 			goto unlock;
>>
>> 		device_init_wakeup(&bdev->pdev->dev, true);
>> +
>> +		pm_runtime_set_autosuspend_delay(&bdev->pdev->dev,
>> +						 BCM_AUTOSUSPEND_DELAY);
>> +		pm_runtime_use_autosuspend(&bdev->pdev->dev);
>> +		pm_runtime_set_active(&bdev->pdev->dev);
>> +		pm_runtime_enable(&bdev->pdev->dev);
>> 	}
>>
>> unlock:
>> @@ -198,7 +214,7 @@ static const struct bcm_set_sleep_mode default_sleep=
_params =3D {
>> 	.bt_wake_active =3D 1,	/* BT_WAKE active mode: 1 =3D high, 0 =3D low */
>> 	.host_wake_active =3D 0,	/* HOST_WAKE active mode: 1 =3D high, 0 =3D lo=
w */
>> 	.allow_host_sleep =3D 1,	/* Allow host sleep in SCO flag */
>> -	.combine_modes =3D 0,	/* Combine sleep and LPM flag */
>> +	.combine_modes =3D 1,	/* Combine sleep and LPM flag */
>> 	.tristate_control =3D 0,	/* Allow tri-state control of UART tx flag */
>> 	/* Irrelevant USB flags */
>> 	.usb_auto_sleep =3D 0,
>> @@ -228,9 +244,28 @@ static int bcm_setup_sleep(struct hci_uart *hu)
>>
>> 	return 0;
>> }
>> +
>> +static void bcm_pm_runtime_work(struct work_struct *work)
>> +{
>> +	struct bcm_data *bcm =3D container_of(work, struct bcm_data, pm_work);
>> +
>> +	mutex_lock(&bcm_device_lock);
>> +	if (bcm_device_exists(bcm->dev)) {
>> +		pm_runtime_get(&bcm->dev->pdev->dev);
>> +		pm_runtime_mark_last_busy(&bcm->dev->pdev->dev);
>> +		pm_runtime_put_autosuspend(&bcm->dev->pdev->dev);
>> +	}
>> +	mutex_unlock(&bcm_device_lock);
>> +}
>> +
>> +static bool bcm_schedule_work(struct bcm_data *bcm)
>> +{
>> +	return schedule_work(&bcm->pm_work);
>> +}
>> #else
>> static inline int bcm_request_irq(struct bcm_data *bcm) { return 0; }
>> static inline int bcm_setup_sleep(struct hci_uart *hu) { return 0; }
>> +static inline bool bcm_schedule_work(struct bcm_data *bcm) { return fal=
se; }
>> #endif
>>
>> static int bcm_open(struct hci_uart *hu)
>> @@ -261,6 +296,7 @@ static int bcm_open(struct hci_uart *hu)
>> 			hu->init_speed =3D dev->init_speed;
>> #ifdef CONFIG_PM
>> 			dev->hu =3D hu;
>> +			INIT_WORK(&bcm->pm_work, bcm_pm_runtime_work);
>> #endif
>> 			bcm_gpio_set_power(bcm->dev, true);
>> 			break;
>> @@ -284,6 +320,11 @@ static int bcm_close(struct hci_uart *hu)
>> 	if (bcm_device_exists(bdev)) {
>> 		bcm_gpio_set_power(bdev, false);
>> #ifdef CONFIG_PM
>> +		cancel_work_sync(&bcm->pm_work);
>> +
>> +		pm_runtime_disable(&bdev->pdev->dev);
>> +		pm_runtime_set_suspended(&bdev->pdev->dev);
>> +
>> 		if (device_can_wakeup(&bdev->pdev->dev)) {
>> 			devm_free_irq(&bdev->pdev->dev, bdev->irq, bdev);
>> 			device_init_wakeup(&bdev->pdev->dev, false);
>> @@ -393,6 +434,13 @@ static int bcm_recv(struct hci_uart *hu, const void=
 *data, int count)
>> 	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
>> 		return -EUNATCH;
>>
>> +	/* mutex_lock/unlock can not be used here as this function is called
>> +	 * from hci_uart_tty_receive under spinlock.
>> +	 * Defer pm_runtime_* calls to work queue
>> +	 */
>> +	if (bcm->dev)
>> +		bcm_schedule_work(bcm);
>> +
>> 	bcm->rx_skb =3D h4_recv_buf(hu->hdev, bcm->rx_skb, data, count,
>> 				  bcm_recv_pkts, ARRAY_SIZE(bcm_recv_pkts));
>> 	if (IS_ERR(bcm->rx_skb)) {
>> @@ -421,8 +469,27 @@ static int bcm_enqueue(struct hci_uart *hu, struct =
sk_buff *skb)
>> static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
>> {
>> 	struct bcm_data *bcm =3D hu->priv;
>> +	struct sk_buff *skb =3D NULL;
>> +	struct bcm_device *bdev =3D NULL;
>> +
>> +	mutex_lock(&bcm_device_lock);
>> +
>> +	if (bcm_device_exists(bcm->dev)) {
>> +		bdev =3D bcm->dev;
>> +		pm_runtime_get_sync(&bdev->pdev->dev);
>> +		/* Shall be resumed here */
>> +	}
>> +
>> +	skb =3D skb_dequeue(&bcm->txq);
>> +
>> +	if (bdev) {
>> +		pm_runtime_mark_last_busy(&bdev->pdev->dev);
>> +		pm_runtime_put_autosuspend(&bdev->pdev->dev);
>> +	}
>>
>> -	return skb_dequeue(&bcm->txq);
>> +	mutex_unlock(&bcm_device_lock);
>> +
>> +	return skb;
>> }
>>
>> #ifdef CONFIG_PM
>> @@ -458,6 +525,34 @@ static void __bcm_resume(struct bcm_device *bdev)
>> 		hci_uart_set_flow_control(bdev->hu, false);
>> 	}
>> }
>> +
>> +static int bcm_runtime_suspend(struct device *dev)
>> +{
>> +	struct bcm_device *bdev =3D platform_get_drvdata(to_platform_device(de=
v));
>> +
>> +	bt_dev_dbg(bdev, "");
>> +
>> +	/* bcm_device_lock held is not required here as bcm_runtime_suspend
>> +	 * is only called when device is attached.
>> +	 */
>> +	__bcm_suspend(bdev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int bcm_runtime_resume(struct device *dev)
>> +{
>> +	struct bcm_device *bdev =3D platform_get_drvdata(to_platform_device(de=
v));
>> +
>> +	bt_dev_dbg(bdev, "");
>> +
>> +	/* bcm_device_lock held is not required here as bcm_runtime_resume
>> +	 * is only called when device is attached.
>> +	 */
>> +	__bcm_resume(bdev);
>> +
>> +	return 0;
>> +}
>> #endif
>>
>> #ifdef CONFIG_PM_SLEEP
>> @@ -474,7 +569,8 @@ static int bcm_suspend(struct device *dev)
>> 	if (!bdev->hu)
>> 		goto unlock;
>>
>> -	__bcm_suspend(bdev);
>> +	if (pm_runtime_active(dev))
>> +		__bcm_suspend(bdev);
>>
>> 	if (device_may_wakeup(&bdev->pdev->dev)) {
>> 		error =3D enable_irq_wake(bdev->irq);
>> @@ -510,6 +606,10 @@ static int bcm_resume(struct device *dev)
>> unlock:
>> 	mutex_unlock(&bcm_device_lock);
>>
>> +	pm_runtime_disable(dev);
>> +	pm_runtime_set_active(dev);
>> +	pm_runtime_enable(dev);
>> +
>> 	return 0;
>> }
>> #endif
>> @@ -726,7 +826,10 @@ MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
>> #endif
>>
>> /* Platform suspend and resume callbacks */
>> -static SIMPLE_DEV_PM_OPS(bcm_pm_ops, bcm_suspend, bcm_resume);
>> +static const struct dev_pm_ops bcm_pm_ops =3D {
>> +	SET_SYSTEM_SLEEP_PM_OPS(bcm_suspend, bcm_resume)
>> +	SET_RUNTIME_PM_OPS(bcm_runtime_suspend, bcm_runtime_resume, NULL)
>
> I think you really need to explain why runtime suspend and system sleep i=
s actually different for Broadcom based devices. Since for the Intel ones i=
t turned out to be the same (at least for now).

I dissociate system sleep and runtime suspend functions for the=20
following reasons:

1) IRQ is enabled as a wake source only during system sleep, while this=20
is not needed for runtime suspend.

IF: The ideal implementation should suspend both the UART and the BT device=
 at runtime. UART should be suspended in a state where the device is flow-c=
ontrolled so the device is unable to send. BT device GPIO based interrupt i=
s then needed for runtime resume initiated by the device. Upon the interrup=
t, your code will resume the UART and allow the device to transmit again. I=
f you leave the UART on w/o flow-controlling the device in suspend you in f=
act don't need that interrupt . However, OEMs will not like that due to the=
 unnecessary UART power consumption. Speaking from experience. :-)

Thanks,
 -Ilya

2) runtime suspend functions do not need to protect usage of dev->hu as=20
these functions are called with a valid dev->hu (these functions are=20
disabled before dev->hu is set to NULL when BCM driver is closed).
System sleep functions need this protection as they can be called even=20
when driver is not opened, and need to ensure that dev->hu is valid=20
until GPIO and UART flow control management is performed.

Common part (GPIO and UART flow control management) for runtime suspend=20
and system sleep moved to __bcm_suspend() and __bcm_resume() in previous=20
patch.

Regards

Fred
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" =
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/4] Bluetooth: hci_bcm: Add suspend/resume runtime PM functions

[Frederic Danis]

Hello Ilya,

On 07/09/2015 23:32, Ilya Faenson wrote:
<snip>
>>> @@ -726,7 +826,10 @@ MODULE_DEVICE_TABLE(acpi, bcm_acpi_match);
>>> >>#endif
>>> >>
>>> >>/* Platform suspend and resume callbacks */
>>> >>-static SIMPLE_DEV_PM_OPS(bcm_pm_ops, bcm_suspend, bcm_resume);
>>> >>+static const struct dev_pm_ops bcm_pm_ops = {
>>> >>+	SET_SYSTEM_SLEEP_PM_OPS(bcm_suspend, bcm_resume)
>>> >>+	SET_RUNTIME_PM_OPS(bcm_runtime_suspend, bcm_runtime_resume, NULL)
>> >
>> >I think you really need to explain why runtime suspend and system sleep is actually different for Broadcom based devices. Since for the Intel ones it turned out to be the same (at least for now).
> I dissociate system sleep and runtime suspend functions for the
> following reasons:
>
> 1) IRQ is enabled as a wake source only during system sleep, while this
> is not needed for runtime suspend.
>
> IF: The ideal implementation should suspend both the UART and the BT device at runtime. UART should be suspended in a state where the device is flow-controlled so the device is unable to send. BT device GPIO based interrupt is then needed for runtime resume initiated by the device. Upon the interrupt, your code will resume the UART and allow the device to transmit again. If you leave the UART on w/o flow-controlling the device in suspend you in fact don't need that interrupt . However, OEMs will not like that due to the unnecessary UART power consumption. Speaking from experience.:-)

Sorry if I was not quite clear enough.

Both bcm_runtime_suspend() and bcm_suspend() enable flow control and 
deassert device_wakeup GPIO in the same way, so that device can resume 
the link via the irq.
The bcm_suspend just adds the capability for the IRQ to wake-up the 
platform (if device_may_wakeup). On resume we disable only this 
capability, meaning that IRQ remains enabled.

>
> Thanks,
>   -Ilya
>
> 2) runtime suspend functions do not need to protect usage of dev->hu as
> these functions are called with a valid dev->hu (these functions are
> disabled before dev->hu is set to NULL when BCM driver is closed).
> System sleep functions need this protection as they can be called even
> when driver is not opened, and need to ensure that dev->hu is valid
> until GPIO and UART flow control management is performed.
>
> Common part (GPIO and UART flow control management) for runtime suspend
> and system sleep moved to __bcm_suspend() and __bcm_resume() in previous
> patch.

Regards

Fred