[PATCH v2 10/10] Bluetooth: hci_bcm: Support Apple GPIO handling

[PATCH v2 10/10] Bluetooth: hci_bcm: Support Apple GPIO handling

[Lukas Wunner]

Enable Bluetooth on the following Macs which provide custom ACPI methods
to toggle the GPIOs for device wake and shutdown instead of accessing
the pins directly:

    MacBook8,1     2015  12"
    MacBook9,1     2016  12"
    MacBook10,1    2017  12"
    MacBookPro13,1 2016  13"
    MacBookPro13,2 2016  13" with Touch Bar
    MacBookPro13,3 2016  15" with Touch Bar
    MacBookPro14,1 2017  13"
    MacBookPro14,2 2017  13" with Touch Bar
    MacBookPro14,3 2017  15" with Touch Bar

On the MacBook8,1 Bluetooth is muxed with a second device (a debug port
on the SSD) under the control of PCH GPIO 36.  Because serdev cannot
deal with multiple slaves yet, it is currently necessary to patch the
DSDT and remove the SSDC device.

The custom ACPI methods are called:

    BTLP (Low Power) takes one argument, toggles device wake GPIO
    BTPU (Power Up) tells SMC to drive shutdown GPIO high
    BTPD (Power Down) tells SMC to drive shutdown GPIO low
    BTRS (Reset) calls BTPD followed by BTPU
    BTRB unknown, not present on all MacBooks

Search for the BTLP, BTPU and BTPD methods on ->probe and cache them in
struct bcm_device if the machine is a Mac.

Additionally, set the init_speed based on a custom device property
provided by Apple in lieu of _CRS resources.  The Broadcom UART's speed
is fixed on Apple Macs:  Any attempt to change it results in Bluetooth
status code 0x0c and bcm_set_baudrate() thus always returns -EBUSY.
By setting only the init_speed and leaving oper_speed at zero, we can
achieve that the host UART's speed is adjusted but the Broadcom UART's
speed is left as is.

The host wake pin goes into the SMC which handles it independently
of the OS, so there's no IRQ for it.

Thanks to Ronald Tschalär who did extensive debugging and testing of
this patch and contributed fixes.

ACPI snippet containing the custom methods and device properties
(taken from a MacBook8,1):

    Method (BTLP, 1, Serialized)
    {
        If (LEqual (Arg0, 0x00))
        {
            Store (0x01, GD54) /* set PCH GPIO 54 direction to input */
        }

        If (LEqual (Arg0, 0x01))
        {
            Store (0x00, GD54) /* set PCH GPIO 54 direction to output */
            Store (0x00, GP54) /* set PCH GPIO 54 value to low */
        }
    }

    Method (BTPU, 0, Serialized)
    {
        Store (0x01, \_SB.PCI0.LPCB.EC.BTPC)
        Sleep (0x0A)
    }

    Method (BTPD, 0, Serialized)
    {
        Store (0x00, \_SB.PCI0.LPCB.EC.BTPC)
        Sleep (0x0A)
    }

    Method (BTRS, 0, Serialized)
    {
        BTPD ()
        BTPU ()
    }

    Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
    {
        If (LEqual (Arg0, ToUUID ("a0b5b7c6-1318-441c-b0c9-fe695eaf949b")))
        {
            Store (Package (0x08)
                {
                    "baud",
                    Buffer (0x08)
                    { 0xC0, 0xC6, 0x2D, 0x00, 0x00, 0x00, 0x00, 0x00 },

                    "parity",
                    Buffer (0x08)
                    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },

                    "dataBits",
                    Buffer (0x08)
                    { 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },

                    "stopBits",
                    Buffer (0x08)
                    { 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 }
                }, Local0)
            DTGP (Arg0, Arg1, Arg2, Arg3, RefOf (Local0))
            Return (Local0)
        }
        Return (0x00)
    }

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=110901
Reported-by: Leif Liddy <leif.liddy@gmail.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Cc: Loic Poulain <loic.poulain@linaro.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Tested-by: Max Shavrick <mxms@me.com>                     [MacBook8,1]
Tested-by: Leif Liddy <leif.liddy@gmail.com>              [MacBook9,1]
Tested-by: Daniel Roschka <danielroschka@phoenitydawn.de> [MacBookPro13,2]
Tested-by: Ronald Tschalär <ronald@innovation.ch>         [MacBookPro13,3]
Tested-by: Peter Y. Chuang <peteryuchuang@gmail.com>      [MacBookPro14,1]
Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
Changes since v1:
  add DSDT excerpt to the commit message,
  drop ternary operators for readability,
  return -EIO instead of -EFAULT if ACPI method calls fail,
  return -EOPNOTSUPP in inline stubs,
  use network subsystem comment style. (Marcel, Hans, Andy)
  Also, to accommodate to mandatory presence of the two GPIOs as per
  patch [2/10], rename bcm_apple_probe() to bcm_apple_get_resources()
  and call it from bcm_get_resources() instead of bcm_acpi_probe().

 drivers/bluetooth/hci_bcm.c | 76 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 74 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index ad6b7c35eb8e..4e0ba38d39d2 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -29,6 +29,7 @@
 #include <linux/acpi.h>
 #include <linux/of.h>
 #include <linux/property.h>
+#include <linux/platform_data/x86/apple.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
 #include <linux/gpio/consumer.h>
@@ -75,6 +76,9 @@
  * @hu: pointer to HCI UART controller struct,
  *	used to enable flow control during runtime suspend and system sleep
  * @is_suspended: whether flow control is currently enabled
+ * @btlp: Apple ACPI method to toggle BT_WAKE pin ("BlueTooth Low Power")
+ * @btpu: Apple ACPI method to drive BT_REG_ON pin high ("BlueTooth Power Up")
+ * @btpd: Apple ACPI method to drive BT_REG_ON pin low ("BlueTooth Power Down")
  */
 struct bcm_device {
 	/* Must be the first member, hci_serdev.c expects this. */
@@ -99,6 +103,9 @@ struct bcm_device {
 	struct hci_uart		*hu;
 	bool			is_suspended;
 #endif
+#ifdef CONFIG_ACPI
+	acpi_handle		btlp, btpu, btpd;
+#endif
 };
 
 /* generic bcm uart resources */
@@ -191,11 +198,59 @@ static bool bcm_device_exists(struct bcm_device *device)
 	return false;
 }
 
+#ifdef CONFIG_ACPI
+static int bcm_apple_get_resources(struct bcm_device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev->dev);
+	const union acpi_object *obj;
+
+	if (!adev ||
+	    ACPI_FAILURE(acpi_get_handle(adev->handle, "BTLP", &dev->btlp)) ||
+	    ACPI_FAILURE(acpi_get_handle(adev->handle, "BTPU", &dev->btpu)) ||
+	    ACPI_FAILURE(acpi_get_handle(adev->handle, "BTPD", &dev->btpd)))
+		return -ENODEV;
+
+	if (!acpi_dev_get_property(adev, "baud", ACPI_TYPE_BUFFER, &obj) &&
+	    obj->buffer.length == 8)
+		dev->init_speed = *(u64 *)obj->buffer.pointer;
+
+	return 0;
+}
+
+static int bcm_apple_set_power(struct bcm_device *dev, bool enable)
+{
+	if (ACPI_FAILURE(acpi_evaluate_object(enable ? dev->btpu : dev->btpd,
+					      NULL, NULL, NULL)))
+		return -EIO;
+
+	return 0;
+}
+
+static int bcm_apple_set_low_power(struct bcm_device *dev, bool enable)
+{
+	if (ACPI_FAILURE(acpi_execute_simple_method(dev->btlp, NULL, enable)))
+		return -EIO;
+
+	return 0;
+}
+#else
+static inline int bcm_apple_get_resources(struct bcm_device *dev)
+{ return -EOPNOTSUPP; }
+static inline int bcm_apple_set_power(struct bcm_device *dev, bool powered)
+{ return -EOPNOTSUPP; }
+static inline int bcm_apple_set_low_power(struct bcm_device *dev, bool enable)
+{ return -EOPNOTSUPP; }
+#endif
+
 static int bcm_gpio_set_device_wakeup(struct bcm_device *dev, bool awake)
 {
 	int err = 0;
 
-	gpiod_set_value(dev->device_wakeup, awake);
+	if (x86_apple_machine)
+		err = bcm_apple_set_low_power(dev, !awake);
+	else
+		gpiod_set_value(dev->device_wakeup, awake);
+
 	bt_dev_dbg(dev, "%s, delaying 15 ms", awake ? "resume" : "suspend");
 	mdelay(15);
 
@@ -212,7 +267,12 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
 			return err;
 	}
 
-	gpiod_set_value(dev->shutdown, powered);
+	if (x86_apple_machine)
+		err = bcm_apple_set_power(dev, powered);
+	else
+		gpiod_set_value(dev->shutdown, powered);
+	if (err)
+		goto err_clk_disable;
 
 	err = bcm_gpio_set_device_wakeup(dev, powered);
 	if (err)
@@ -227,6 +287,7 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
 
 err_revert_shutdown:
 	gpiod_set_value(dev->shutdown, !powered);
+err_clk_disable:
 	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
 		clk_disable_unprepare(dev->clk);
 	return err;
@@ -255,6 +316,14 @@ static int bcm_request_irq(struct bcm_data *bcm)
 		goto unlock;
 	}
 
+	/* Macs wire the host wake pin to the System Management Controller,
+	 * which handles wakeup independently of the operating system.
+	 */
+	if (x86_apple_machine) {
+		err = 0;
+		goto unlock;
+	}
+
 	if (bdev->irq <= 0) {
 		err = -EOPNOTSUPP;
 		goto unlock;
@@ -846,6 +915,9 @@ static int bcm_get_resources(struct bcm_device *dev)
 {
 	dev->name = dev_name(dev->dev);
 
+	if (x86_apple_machine && !bcm_apple_get_resources(dev))
+		return 0;
+
 	dev->clk = devm_clk_get(dev->dev, NULL);
 
 	dev->device_wakeup = devm_gpiod_get(dev->dev, "device-wakeup",
-- 
2.15.1

[PATCH v2 09/10] Bluetooth: hci_bcm: Handle errors properly

[Lukas Wunner]

A significant portion of this driver lacks error handling.  As a first
step, add error paths to bcm_gpio_set_power(), bcm_open(), bcm_close(),
bcm_suspend_device(), bcm_resume_device(), bcm_resume(), bcm_probe() and
bcm_serdev_probe().  (I've also scrutinized bcm_suspend() but think it's
fine as is.)

Those are all the functions accessing the device wake and shutdown GPIO.
On Apple Macs the pins are accessed through ACPI methods, which may fail
for various reasons, hence proper error handling is necessary.  Non-Macs
access the pins directly, which may fail as well but the GPIO core does
not yet pass back errors to consumers.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 84 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 69 insertions(+), 15 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 30ac688dc2c7..ad6b7c35eb8e 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -204,12 +204,19 @@ static int bcm_gpio_set_device_wakeup(struct bcm_device *dev, bool awake)
 
 static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
 {
-	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
-		clk_prepare_enable(dev->clk);
+	int err = 0;
+
+	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled) {
+		err = clk_prepare_enable(dev->clk);
+		if (err)
+			return err;
+	}
 
 	gpiod_set_value(dev->shutdown, powered);
 
-	bcm_gpio_set_device_wakeup(dev, powered);
+	err = bcm_gpio_set_device_wakeup(dev, powered);
+	if (err)
+		goto err_revert_shutdown;
 
 	if (!powered && !IS_ERR(dev->clk) && dev->clk_enabled)
 		clk_disable_unprepare(dev->clk);
@@ -217,6 +224,12 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
 	dev->clk_enabled = powered;
 
 	return 0;
+
+err_revert_shutdown:
+	gpiod_set_value(dev->shutdown, !powered);
+	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
+		clk_disable_unprepare(dev->clk);
+	return err;
 }
 
 #ifdef CONFIG_PM
@@ -331,6 +344,7 @@ static int bcm_open(struct hci_uart *hu)
 {
 	struct bcm_data *bcm;
 	struct list_head *p;
+	int err;
 
 	bt_dev_dbg(hu->hdev, "hu %p", hu);
 
@@ -345,7 +359,10 @@ static int bcm_open(struct hci_uart *hu)
 	mutex_lock(&bcm_device_lock);
 
 	if (hu->serdev) {
-		serdev_device_open(hu->serdev);
+		err = serdev_device_open(hu->serdev);
+		if (err)
+			goto err_free;
+
 		bcm->dev = serdev_device_get_drvdata(hu->serdev);
 		goto out;
 	}
@@ -373,17 +390,30 @@ static int bcm_open(struct hci_uart *hu)
 	if (bcm->dev) {
 		hu->init_speed = bcm->dev->init_speed;
 		hu->oper_speed = bcm->dev->oper_speed;
-		bcm_gpio_set_power(bcm->dev, true);
+		err = bcm_gpio_set_power(bcm->dev, true);
+		if (err)
+			goto err_unset_hu;
 	}
 
 	mutex_unlock(&bcm_device_lock);
 	return 0;
+
+err_unset_hu:
+#ifdef CONFIG_PM
+	bcm->dev->hu = NULL;
+#endif
+err_free:
+	mutex_unlock(&bcm_device_lock);
+	hu->priv = NULL;
+	kfree(bcm);
+	return err;
 }
 
 static int bcm_close(struct hci_uart *hu)
 {
 	struct bcm_data *bcm = hu->priv;
 	struct bcm_device *bdev = NULL;
+	int err;
 
 	bt_dev_dbg(hu->hdev, "hu %p", hu);
 
@@ -401,9 +431,13 @@ static int bcm_close(struct hci_uart *hu)
 	}
 
 	if (bdev) {
-		bcm_gpio_set_power(bdev, false);
-		pm_runtime_disable(bdev->dev);
-		pm_runtime_set_suspended(bdev->dev);
+		err = bcm_gpio_set_power(bdev, false);
+		if (err) {
+			bt_dev_err(hu->hdev, "Failed to power down");
+		} else {
+			pm_runtime_disable(bdev->dev);
+			pm_runtime_set_suspended(bdev->dev);
+		}
 
 		if (IS_ENABLED(CONFIG_PM) && bdev->irq > 0) {
 			devm_free_irq(bdev->dev, bdev->irq, bdev);
@@ -593,6 +627,7 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
 static int bcm_suspend_device(struct device *dev)
 {
 	struct bcm_device *bdev = dev_get_drvdata(dev);
+	int err;
 
 	bt_dev_dbg(bdev, "");
 
@@ -604,7 +639,14 @@ static int bcm_suspend_device(struct device *dev)
 	}
 
 	/* Suspend the device */
-	bcm_gpio_set_device_wakeup(bdev, false);
+	err = bcm_gpio_set_device_wakeup(bdev, false);
+	if (err) {
+		if (bdev->is_suspended && bdev->hu) {
+			bdev->is_suspended = false;
+			hci_uart_set_flow_control(bdev->hu, false);
+		}
+		return -EBUSY;
+	}
 
 	return 0;
 }
@@ -612,10 +654,15 @@ static int bcm_suspend_device(struct device *dev)
 static int bcm_resume_device(struct device *dev)
 {
 	struct bcm_device *bdev = dev_get_drvdata(dev);
+	int err;
 
 	bt_dev_dbg(bdev, "");
 
-	bcm_gpio_set_device_wakeup(bdev, true);
+	err = bcm_gpio_set_device_wakeup(bdev, true);
+	if (err) {
+		dev_err(dev, "Failed to power up\n");
+		return err;
+	}
 
 	/* When this executes, the device has woken up already */
 	if (bdev->is_suspended && bdev->hu) {
@@ -667,6 +714,7 @@ static int bcm_suspend(struct device *dev)
 static int bcm_resume(struct device *dev)
 {
 	struct bcm_device *bdev = dev_get_drvdata(dev);
+	int err = 0;
 
 	bt_dev_dbg(bdev, "resume: is_suspended %d", bdev->is_suspended);
 
@@ -686,14 +734,16 @@ static int bcm_resume(struct device *dev)
 		bt_dev_dbg(bdev, "BCM irq: disabled");
 	}
 
-	bcm_resume_device(dev);
+	err = bcm_resume_device(dev);
 
 unlock:
 	mutex_unlock(&bcm_device_lock);
 
-	pm_runtime_disable(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
+	if (!err) {
+		pm_runtime_disable(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+	}
 
 	return 0;
 }
@@ -909,7 +959,9 @@ static int bcm_probe(struct platform_device *pdev)
 	list_add_tail(&dev->list, &bcm_device_list);
 	mutex_unlock(&bcm_device_lock);
 
-	bcm_gpio_set_power(dev, false);
+	ret = bcm_gpio_set_power(dev, false);
+	if (ret)
+		dev_err(&pdev->dev, "Failed to power down\n");
 
 	return 0;
 }
@@ -1012,6 +1064,8 @@ static int bcm_serdev_probe(struct serdev_device *serdev)
 		return err;
 
 	bcm_gpio_set_power(bcmdev, false);
+	if (err)
+		dev_err(&serdev->dev, "Failed to power down\n");
 
 	return hci_uart_register_device(&bcmdev->serdev_hu, &bcm_proto);
 }
-- 
2.15.1

[PATCH v2 08/10] Bluetooth: hci_bcm: Document struct bcm_device

[Lukas Wunner]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index dbf9d10dadf1..30ac688dc2c7 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -52,7 +52,30 @@
 
 #define BCM_AUTOSUSPEND_DELAY	5000 /* default autosleep delay */
 
-/* device driver resources */
+/**
+ * struct bcm_device - device driver resources
+ * @serdev_hu: HCI UART controller struct
+ * @list: bcm_device_list node
+ * @dev: physical UART slave
+ * @name: device name logged by bt_dev_*() functions
+ * @device_wakeup: BT_WAKE pin,
+ *	assert = Bluetooth device must wake up or remain awake,
+ *	deassert = Bluetooth device may sleep when sleep criteria are met
+ * @shutdown: BT_REG_ON pin,
+ *	power up or power down Bluetooth device internal regulators
+ * @clk: clock used by Bluetooth device
+ * @clk_enabled: whether @clk is prepared and enabled
+ * @init_speed: default baudrate of Bluetooth device;
+ *	the host UART is initially set to this baudrate so that
+ *	it can configure the Bluetooth device for @oper_speed
+ * @oper_speed: preferred baudrate of Bluetooth device;
+ *	set to 0 if @init_speed is already the preferred baudrate
+ * @irq: interrupt triggered by HOST_WAKE_BT pin
+ * @irq_active_low: whether @irq is active low
+ * @hu: pointer to HCI UART controller struct,
+ *	used to enable flow control during runtime suspend and system sleep
+ * @is_suspended: whether flow control is currently enabled
+ */
 struct bcm_device {
 	/* Must be the first member, hci_serdev.c expects this. */
 	struct hci_uart		serdev_hu;
@@ -74,7 +97,7 @@ struct bcm_device {
 
 #ifdef CONFIG_PM
 	struct hci_uart		*hu;
-	bool			is_suspended; /* suspend/resume flag */
+	bool			is_suspended;
 #endif
 };
 
-- 
2.15.1

[PATCH v2 07/10] Bluetooth: hci_bcm: Clean up unnecessary #ifdef

[Lukas Wunner]

pm_runtime_disable() and pm_runtime_set_suspended() are replaced with
empty inlines if CONFIG_PM is disabled, so there's no need to #ifdef
them.

device_init_wakeup() is likewise replaced with an inline, though it's
not empty, but it and devm_free_irq() can be made conditional on
IS_ENABLED(CONFIG_PM), which is preferable to #ifdef as per section 20
of Documentation/process/coding-style.rst.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 632939d413df..dbf9d10dadf1 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -379,15 +379,13 @@ static int bcm_close(struct hci_uart *hu)
 
 	if (bdev) {
 		bcm_gpio_set_power(bdev, false);
-#ifdef CONFIG_PM
 		pm_runtime_disable(bdev->dev);
 		pm_runtime_set_suspended(bdev->dev);
 
-		if (bdev->irq > 0) {
+		if (IS_ENABLED(CONFIG_PM) && bdev->irq > 0) {
 			devm_free_irq(bdev->dev, bdev->irq, bdev);
 			device_init_wakeup(bdev->dev, false);
 		}
-#endif
 	}
 	mutex_unlock(&bcm_device_lock);
 
-- 
2.15.1

[PATCH v2 06/10] Bluetooth: hci_bcm: Silence IRQ printk

[Lukas Wunner]

The host wake IRQ is optional, but if none is found, "BCM irq: -22" is
logged which may irritate users.  This is really a debug message, so use
dev_dbg() instead of dev_info().  If users are interested in the IRQ,
they can always consult /proc/interrupts.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 01d0943b7bbb..632939d413df 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -798,7 +798,7 @@ static int bcm_get_resources(struct bcm_device *dev)
 		dev->irq = gpiod_to_irq(gpio);
 	}
 
-	dev_info(dev->dev, "BCM irq: %d\n", dev->irq);
+	dev_dbg(dev->dev, "BCM irq: %d\n", dev->irq);
 	return 0;
 }
 
-- 
2.15.1

[PATCH v2 05/10] Bluetooth: hci_bcm: Add helper to toggle device wake GPIO

[Lukas Wunner]

There is already a bcm_gpio_set_power() helper to toggle the power
enable pin.  Add a corresponding helper to toggle the device wake pin.
This will allow us to more easily integrate support for MacBooks which
use custom ACPI methods to access both pins.

The only functional change here is an additional 15 ms delay when
toggling the device wake pin in bcm_gpio_set_power().  It is unclear why
this delay is observed in bcm_suspend_device() and bcm_resume_device(),
but not in bcm_gpio_set_power() so far.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
Changes since v1:
  I had previously forgotten to toggle the device wake
  GPIO in bcm_gpio_set_power() on Macs, this is now fixed.

 drivers/bluetooth/hci_bcm.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index dbba08fd05a3..01d0943b7bbb 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -168,13 +168,25 @@ static bool bcm_device_exists(struct bcm_device *device)
 	return false;
 }
 
+static int bcm_gpio_set_device_wakeup(struct bcm_device *dev, bool awake)
+{
+	int err = 0;
+
+	gpiod_set_value(dev->device_wakeup, awake);
+	bt_dev_dbg(dev, "%s, delaying 15 ms", awake ? "resume" : "suspend");
+	mdelay(15);
+
+	return err;
+}
+
 static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
 {
 	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
 		clk_prepare_enable(dev->clk);
 
 	gpiod_set_value(dev->shutdown, powered);
-	gpiod_set_value(dev->device_wakeup, powered);
+
+	bcm_gpio_set_device_wakeup(dev, powered);
 
 	if (!powered && !IS_ERR(dev->clk) && dev->clk_enabled)
 		clk_disable_unprepare(dev->clk);
@@ -571,9 +583,7 @@ static int bcm_suspend_device(struct device *dev)
 	}
 
 	/* Suspend the device */
-	gpiod_set_value(bdev->device_wakeup, false);
-	bt_dev_dbg(bdev, "suspend, delaying 15 ms");
-	mdelay(15);
+	bcm_gpio_set_device_wakeup(bdev, false);
 
 	return 0;
 }
@@ -584,9 +594,7 @@ static int bcm_resume_device(struct device *dev)
 
 	bt_dev_dbg(bdev, "");
 
-	gpiod_set_value(bdev->device_wakeup, true);
-	bt_dev_dbg(bdev, "resume, delaying 15 ms");
-	mdelay(15);
+	bcm_gpio_set_device_wakeup(bdev, true);
 
 	/* When this executes, the device has woken up already */
 	if (bdev->is_suspended && bdev->hu) {
-- 
2.15.1

[PATCH v2 04/10] Bluetooth: hci_bcm: Validate IRQ before using it

[Lukas Wunner]

From: Ronald Tschalär <ronald@innovation.ch>

The ->close, ->suspend and ->resume hooks assume presence of a valid IRQ
if the device is wakeup capable.  However it's entirely possible that
wakeup was enabled by some other entity besides this driver and in this
case the user will get a WARN splat if no valid IRQ was found.  Avoid by
checking if the IRQ is valid, i.e. > 0.

Case in point:  On recent MacBook Pros, the Bluetooth device lacks an
IRQ (because host wakeup is handled by the SMC, independently of the
operating system), but it does possess a _PRW method (which specifies
the SMC's GPE as wake event).  The ACPI core therefore automatically
marks the physical Bluetooth device wakeup capable upon binding it to
its ACPI companion:

device_set_wakeup_capable+0x96/0xb0
acpi_bind_one+0x28a/0x310
acpi_platform_notify+0x20/0xa0
device_add+0x215/0x690
serdev_device_add+0x57/0xf0
acpi_serdev_add_device+0xc9/0x110
acpi_ns_walk_namespace+0x131/0x280
acpi_walk_namespace+0xf5/0x13d
serdev_controller_add+0x6f/0x110
serdev_tty_port_register+0x98/0xf0
tty_port_register_device_attr_serdev+0x3a/0x70
uart_add_one_port+0x268/0x500
serial8250_register_8250_port+0x32e/0x490
dw8250_probe+0x46c/0x720
platform_drv_probe+0x35/0x90
driver_probe_device+0x300/0x450
bus_for_each_drv+0x67/0xb0
__device_attach+0xde/0x160
bus_probe_device+0x9c/0xb0
device_add+0x448/0x690
platform_device_add+0x10e/0x260
mfd_add_device+0x392/0x4c0
mfd_add_devices+0xb1/0x110
intel_lpss_probe+0x2a9/0x610 [intel_lpss]
intel_lpss_pci_probe+0x7a/0xa8 [intel_lpss_pci]

Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
[lukas: fix up ->suspend and ->resume as well, add commit message]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index d9ca27d3209a..dbba08fd05a3 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -371,7 +371,7 @@ static int bcm_close(struct hci_uart *hu)
 		pm_runtime_disable(bdev->dev);
 		pm_runtime_set_suspended(bdev->dev);
 
-		if (device_can_wakeup(bdev->dev)) {
+		if (bdev->irq > 0) {
 			devm_free_irq(bdev->dev, bdev->irq, bdev);
 			device_init_wakeup(bdev->dev, false);
 		}
@@ -622,7 +622,7 @@ static int bcm_suspend(struct device *dev)
 	if (pm_runtime_active(dev))
 		bcm_suspend_device(dev);
 
-	if (device_may_wakeup(dev)) {
+	if (device_may_wakeup(dev) && bdev->irq > 0) {
 		error = enable_irq_wake(bdev->irq);
 		if (!error)
 			bt_dev_dbg(bdev, "BCM irq: enabled");
@@ -652,7 +652,7 @@ static int bcm_resume(struct device *dev)
 	if (!bdev->hu)
 		goto unlock;
 
-	if (device_may_wakeup(dev)) {
+	if (device_may_wakeup(dev) && bdev->irq > 0) {
 		disable_irq_wake(bdev->irq);
 		bt_dev_dbg(bdev, "BCM irq: disabled");
 	}
-- 
2.15.1

[PATCH v2 03/10] Bluetooth: hci_bcm: Enable runtime PM despite absence of IRQ

[Lukas Wunner]

Currently runtime PM is only enabled if a valid host wake IRQ was found.

However it is used in ways which seem unrelated to host wake:
E.g., runtime PM is used to force the Bluetooth device on for 5 sec
after a complete packet has been received.

Afford this functionality to devices which lack an IRQ by moving the
code to enable runtime PM from bcm_request_irq() to bcm_setup().

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 4294d9df1d4d..d9ca27d3209a 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -221,12 +221,6 @@ static int bcm_request_irq(struct bcm_data *bcm)
 
 	device_init_wakeup(bdev->dev, true);
 
-	pm_runtime_set_autosuspend_delay(bdev->dev,
-					 BCM_AUTOSUSPEND_DELAY);
-	pm_runtime_use_autosuspend(bdev->dev);
-	pm_runtime_set_active(bdev->dev);
-	pm_runtime_enable(bdev->dev);
-
 unlock:
 	mutex_unlock(&bcm_device_lock);
 
@@ -468,6 +462,11 @@ static int bcm_setup(struct hci_uart *hu)
 	if (!bcm_request_irq(bcm))
 		err = bcm_setup_sleep(hu);
 
+	pm_runtime_set_autosuspend_delay(bcm->dev->dev, BCM_AUTOSUSPEND_DELAY);
+	pm_runtime_use_autosuspend(bcm->dev->dev);
+	pm_runtime_set_active(bcm->dev->dev);
+	pm_runtime_enable(bcm->dev->dev);
+
 	return err;
 }
 
-- 
2.15.1

[PATCH v2 02/10] Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO

[Lukas Wunner]

Commit 0395ffc1ee05 ("Bluetooth: hci_bcm: Add PM for BCM devices")
amended this driver to request a shutdown and device wake GPIO on probe,
but mandated that only one of them need to be present:

	/* Make sure at-least one of the GPIO is defined and that
	 * a name is specified for this instance
	 */
	if ((!dev->device_wakeup && !dev->shutdown) || !dev->name) {
		dev_err(&pdev->dev, "invalid platform data\n");
		return -EINVAL;
	}

However the same commit added a call to bcm_gpio_set_power() to the
->probe hook, which unconditionally accesses *both* GPIOs.  Luckily,
the resulting NULL pointer deref was never reported, suggesting there's
no machine where either GPIO is missing.

Commit 8a92056837fd ("Bluetooth: hci_bcm: Add (runtime)pm support to the
serdev driver") removed the check whether at least one of the GPIOs is
present without specifying a reason.

Because commit 62aaefa7d038 ("Bluetooth: hci_bcm: improve use of gpios
API") refactored the driver to use devm_gpiod_get_optional() instead of
devm_gpiod_get(), one is now tempted to believe that the driver doesn't
require *any* of the two GPIOs.

Which is wrong, the driver still requires both GPIOs to avoid a NULL
pointer deref.  To this end, establish the status quo ante and request
the GPIOs with devm_gpiod_get() again.  Bail out of ->probe if either
of them is missing.

Oddly enough, whereas bcm_gpio_set_power() accesses the device wake pin
unconditionally, bcm_suspend_device() and bcm_resume_device() do check
for its presence before accessing it.  Those checks are superfluous,
so remove them.

Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
Cc: Loic Poulain <loic.poulain@linaro.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/hci_bcm.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 1fc604a0d870..4294d9df1d4d 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -572,11 +572,9 @@ static int bcm_suspend_device(struct device *dev)
 	}
 
 	/* Suspend the device */
-	if (bdev->device_wakeup) {
-		gpiod_set_value(bdev->device_wakeup, false);
-		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
-		mdelay(15);
-	}
+	gpiod_set_value(bdev->device_wakeup, false);
+	bt_dev_dbg(bdev, "suspend, delaying 15 ms");
+	mdelay(15);
 
 	return 0;
 }
@@ -587,11 +585,9 @@ static int bcm_resume_device(struct device *dev)
 
 	bt_dev_dbg(bdev, "");
 
-	if (bdev->device_wakeup) {
-		gpiod_set_value(bdev->device_wakeup, true);
-		bt_dev_dbg(bdev, "resume, delaying 15 ms");
-		mdelay(15);
-	}
+	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) {
@@ -774,14 +770,12 @@ static int bcm_get_resources(struct bcm_device *dev)
 
 	dev->clk = devm_clk_get(dev->dev, NULL);
 
-	dev->device_wakeup = devm_gpiod_get_optional(dev->dev,
-						     "device-wakeup",
-						     GPIOD_OUT_LOW);
+	dev->device_wakeup = devm_gpiod_get(dev->dev, "device-wakeup",
+							     GPIOD_OUT_LOW);
 	if (IS_ERR(dev->device_wakeup))
 		return PTR_ERR(dev->device_wakeup);
 
-	dev->shutdown = devm_gpiod_get_optional(dev->dev, "shutdown",
-						GPIOD_OUT_LOW);
+	dev->shutdown = devm_gpiod_get(dev->dev, "shutdown", GPIOD_OUT_LOW);
 	if (IS_ERR(dev->shutdown))
 		return PTR_ERR(dev->shutdown);
 
-- 
2.15.1

[PATCH v2 01/10] Bluetooth: Depend on rather than select GPIOLIB

[Lukas Wunner]

Commit 27378f4c1b92 ("Bluetooth: Avoid WARN splat due to missing
GPIOLIB") amended Kconfig to select GPIOLIB if BT_HCIUART_NOKIA,
BT_HCIUART_INTEL or BT_HCIUART_BCM is enabled since all three drivers
require it to function.

The diagnosis was correct but the treatment was not.  As stated in
Documentation/gpio/consumer.txt:

    Guidelines for GPIOs consumers
    ==============================

    Drivers that can't work without standard GPIO calls should have
    Kconfig entries that depend on GPIOLIB.
                         ^^^^^^^^^
Fix it.

Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/bluetooth/Kconfig | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index d816ce9e23a7..07e55cd8f8c8 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -108,10 +108,10 @@ config BT_HCIUART_NOKIA
 	tristate "UART Nokia H4+ protocol support"
 	depends on BT_HCIUART
 	depends on BT_HCIUART_SERDEV
+	depends on GPIOLIB
 	depends on PM
 	select BT_HCIUART_H4
 	select BT_BCM
-	select GPIOLIB
 	help
 	  Nokia H4+ is serial protocol for communication between Bluetooth
 	  device and host. This protocol is required for Bluetooth devices
@@ -170,9 +170,9 @@ config BT_HCIUART_3WIRE
 config BT_HCIUART_INTEL
 	bool "Intel protocol support"
 	depends on BT_HCIUART
+	depends on GPIOLIB
 	select BT_HCIUART_H4
 	select BT_INTEL
-	select GPIOLIB
 	help
 	  The Intel protocol support enables Bluetooth HCI over serial
 	  port interface for Intel Bluetooth controllers.
@@ -184,9 +184,9 @@ config BT_HCIUART_BCM
 	depends on BT_HCIUART
 	depends on BT_HCIUART_SERDEV
 	depends on (!ACPI || SERIAL_DEV_CTRL_TTYPORT)
+	depends on GPIOLIB
 	select BT_HCIUART_H4
 	select BT_BCM
-	select GPIOLIB
 	help
 	  The Broadcom protocol support enables Bluetooth HCI over serial
 	  port interface for Broadcom Bluetooth controllers.
-- 
2.15.1

[PATCH v2 00/10] Bluetooth on 2015+ MacBook (Pro)

[Lukas Wunner]

Enable UART-attached Bluetooth on 2015+ MacBook (Pro), v2.

Changes since v1:

- New patch [1/10] to make Bluetooth drivers depend on rather than select
  GPIOLIB. (Andy, Linus)

- New patch [2/10] to enforce presence of shutdown and device wake GPIO.
  (Andy, Linus, Loic)

- Split off hunks:
  to enable runtime PM despite absence of IRQ into patch [3/10],
  to validate the IRQ before using it into patch [4/10],
  to add a helper to toggle device wake GPIO into patch [5/10]. (Marcel)

- In patch [5/10], I had previously forgotten to toggle the device wake
  GPIO in bcm_gpio_set_power() on Macs, this is now fixed.

- New patch [6/10] to silence an irritating IRQ printk.

- New patch [7/10] to clean up an unnecessary #ifdef.

- New patch [8/10] to add kerneldoc for struct bcm_device.

- New patch [9/10] to add proper error handling to the driver.

- In patch [10/10]:
  add DSDT excerpt to the commit message,
  drop ternary operators for readability,
  return -EIO instead of -EFAULT if ACPI method calls fail,
  return -EOPNOTSUPP in inline stubs,
  use network subsystem comment style. (Marcel, Hans, Andy)
  Also, to accommodate to mandatory presence of the two GPIOs as per
  patch [2/10], rename bcm_apple_probe() to bcm_apple_get_resources()
  and call it from bcm_get_resources() instead of bcm_acpi_probe().

Link to v1:

https://www.spinics.net/lists/linux-bluetooth/msg73527.html

Thanks,

Lukas


Lukas Wunner (9):
  Bluetooth: Depend on rather than select GPIOLIB
  Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO
  Bluetooth: hci_bcm: Enable runtime PM despite absence of IRQ
  Bluetooth: hci_bcm: Add helper to toggle device wake GPIO
  Bluetooth: hci_bcm: Silence IRQ printk
  Bluetooth: hci_bcm: Clean up unnecessary #ifdef
  Bluetooth: hci_bcm: Document struct bcm_device
  Bluetooth: hci_bcm: Handle errors properly
  Bluetooth: hci_bcm: Support Apple GPIO handling

Ronald Tschalär (1):
  Bluetooth: hci_bcm: Validate IRQ before using it

 drivers/bluetooth/Kconfig   |   6 +-
 drivers/bluetooth/hci_bcm.c | 230 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 192 insertions(+), 44 deletions(-)

-- 
2.15.1

Re: [PATCH v2 03/10] Bluetooth: hci_bcm: Enable runtime PM despite absence of IRQ

[Hans de Goede]

Hi,

On 02-01-18 20:08, Lukas Wunner wrote:
> Currently runtime PM is only enabled if a valid host wake IRQ was found.
> 
> However it is used in ways which seem unrelated to host wake:
> E.g., runtime PM is used to force the Bluetooth device on for 5 sec
> after a complete packet has been received.
> 
> Afford this functionality to devices which lack an IRQ by moving the
> code to enable runtime PM from bcm_request_irq() to bcm_setup().
> 

Hmm, this seems to not make much sense. If runtime_pm is not enabled
(no host wake irq) then the device never runtime suspends, so there
is no need to force the device on when we're sending / after a
complete packet has been received.

That you give "after a complete packet has been received" as condition
example illustrates nicely why I believe this commit is a bad idea,
if say a bluetooth keyboard wants to send a keypress HID report after
the runtime pm timeout has elapsed, how are we going to receive that
report since we're runtime suspended and there is no host wake irq
to tell us to wakeup the UART and enable receiving on it again?

I don't see how runtime pm can work without a host-wake IRQ,
so IMHO this commit is wrong. Tip when testing this, make sure
that you don't have the bluetooth config panel of e.g. gnome3
open as that continuously scans for new devices, so runtime pm
never kicks in. I've had a bad configured host IRQ pin on some
ASUS laptops using serdev bcm bt and the only way a bt keyboard
would work reliable was to keep the config panel open...

Regards,

Hans




> Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de > ---
>   drivers/bluetooth/hci_bcm.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 4294d9df1d4d..d9ca27d3209a 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -221,12 +221,6 @@ static int bcm_request_irq(struct bcm_data *bcm)
>   
>   	device_init_wakeup(bdev->dev, true);
>   
> -	pm_runtime_set_autosuspend_delay(bdev->dev,
> -					 BCM_AUTOSUSPEND_DELAY);
> -	pm_runtime_use_autosuspend(bdev->dev);
> -	pm_runtime_set_active(bdev->dev);
> -	pm_runtime_enable(bdev->dev);
> -
>   unlock:
>   	mutex_unlock(&bcm_device_lock);
>   
> @@ -468,6 +462,11 @@ static int bcm_setup(struct hci_uart *hu)
>   	if (!bcm_request_irq(bcm))
>   		err = bcm_setup_sleep(hu);
>   
> +	pm_runtime_set_autosuspend_delay(bcm->dev->dev, BCM_AUTOSUSPEND_DELAY);
> +	pm_runtime_use_autosuspend(bcm->dev->dev);
> +	pm_runtime_set_active(bcm->dev->dev);
> +	pm_runtime_enable(bcm->dev->dev);
> +
>   	return err;
>   }
>   
> 

Re: [PATCH v2 03/10] Bluetooth: hci_bcm: Enable runtime PM despite absence of IRQ

[Lukas Wunner]

On Tue, Jan 02, 2018 at 08:17:24PM +0100, Hans de Goede wrote:
> On 02-01-18 20:08, Lukas Wunner wrote:
> > Currently runtime PM is only enabled if a valid host wake IRQ was found.
> > 
> > However it is used in ways which seem unrelated to host wake:
> > E.g., runtime PM is used to force the Bluetooth device on for 5 sec
> > after a complete packet has been received.
> > 
> > Afford this functionality to devices which lack an IRQ by moving the
> > code to enable runtime PM from bcm_request_irq() to bcm_setup().
> 
> Hmm, this seems to not make much sense. If runtime_pm is not enabled
> (no host wake irq) then the device never runtime suspends, so there
> is no need to force the device on when we're sending / after a
> complete packet has been received.
> 
> That you give "after a complete packet has been received" as condition
> example illustrates nicely why I believe this commit is a bad idea,
> if say a bluetooth keyboard wants to send a keypress HID report after
> the runtime pm timeout has elapsed, how are we going to receive that
> report since we're runtime suspended and there is no host wake irq
> to tell us to wakeup the UART and enable receiving on it again?

The host UART on MacBooks apparently never runtime suspends, so this
couldn't have been observed during testing:

They use either 8250_lpss.c (which has no runtime PM callbacks)
or 8250_dw.c (which disables the clock during runtime suspend,
but the MacBooks use a fixed rate clock which can't be disabled).
Moreover intel_lpss_suspend() excludes the UART from being put
into reset.

But the objection is valid so I withdraw this patch.

On MacBooks the host wake pin is ORed with the wake pin of the trackpad
and keyboard and goes into the SMC.  I think it is only meant to be
used to wake the system from sleep and not to provide an interrupt
at runtime.  In theory we could request the SMC's GPE but we couldn't
tell if the interrupt was sent by the trackpad/keyboard or by the
Bluetooth controller, or was caused by something else entirely.

So it's probably better not to use runtime PM at all.  Let me rework
patch [10/10] and remove this hunk:

+	/* Macs wire the host wake pin to the System Management Controller,
+	 * which handles wakeup independently of the operating system.
+	 */
+	if (x86_apple_machine) {
+		err = 0;
+		goto unlock;
+	}
+

I notice that command 0xfc27 is used to write the sleep_params.
Does anyone know the command to read them so that we can check the
defaults Apple is using and find out if they can be optimized?


> I don't see how runtime pm can work without a host-wake IRQ,
> so IMHO this commit is wrong. Tip when testing this, make sure
> that you don't have the bluetooth config panel of e.g. gnome3
> open as that continuously scans for new devices, so runtime pm
> never kicks in. I've had a bad configured host IRQ pin on some
> ASUS laptops using serdev bcm bt and the only way a bt keyboard
> would work reliable was to keep the config panel open...

I don't have one of these MacBooks, I'm submitting patches on
behalf of others, but I'm told that having the gnome panel open
leads to choppy audio on these machines.  Is that normal?
The Bluetooth controllers are fixed to a baudrate of 3 Mbit
on these machines.

Thanks,

Lukas

Re: [PATCH v2 01/10] Bluetooth: Depend on rather than select GPIOLIB

[Linus Walleij]

On Tue, Jan 2, 2018 at 8:08 PM, Lukas Wunner <lukas@wunner.de> wrote:

> Commit 27378f4c1b92 ("Bluetooth: Avoid WARN splat due to missing
> GPIOLIB") amended Kconfig to select GPIOLIB if BT_HCIUART_NOKIA,
> BT_HCIUART_INTEL or BT_HCIUART_BCM is enabled since all three drivers
> require it to function.
>
> The diagnosis was correct but the treatment was not.  As stated in
> Documentation/gpio/consumer.txt:
>
>     Guidelines for GPIOs consumers
>     ==============================
>
>     Drivers that can't work without standard GPIO calls should have
>     Kconfig entries that depend on GPIOLIB.
>                          ^^^^^^^^^
> Fix it.
>
> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 03/10] Bluetooth: hci_bcm: Enable runtime PM despite absence of IRQ

[Hans de Goede]

Hi,

On 03-01-18 00:36, Lukas Wunner wrote:
> On Tue, Jan 02, 2018 at 08:17:24PM +0100, Hans de Goede wrote:
>> On 02-01-18 20:08, Lukas Wunner wrote:
>>> Currently runtime PM is only enabled if a valid host wake IRQ was found.
>>>
>>> However it is used in ways which seem unrelated to host wake:
>>> E.g., runtime PM is used to force the Bluetooth device on for 5 sec
>>> after a complete packet has been received.
>>>
>>> Afford this functionality to devices which lack an IRQ by moving the
>>> code to enable runtime PM from bcm_request_irq() to bcm_setup().
>>
>> Hmm, this seems to not make much sense. If runtime_pm is not enabled
>> (no host wake irq) then the device never runtime suspends, so there
>> is no need to force the device on when we're sending / after a
>> complete packet has been received.
>>
>> That you give "after a complete packet has been received" as condition
>> example illustrates nicely why I believe this commit is a bad idea,
>> if say a bluetooth keyboard wants to send a keypress HID report after
>> the runtime pm timeout has elapsed, how are we going to receive that
>> report since we're runtime suspended and there is no host wake irq
>> to tell us to wakeup the UART and enable receiving on it again?
> 
> The host UART on MacBooks apparently never runtime suspends, so this
> couldn't have been observed during testing:
> 
> They use either 8250_lpss.c (which has no runtime PM callbacks)
> or 8250_dw.c (which disables the clock during runtime suspend,
> but the MacBooks use a fixed rate clock which can't be disabled).
> Moreover intel_lpss_suspend() excludes the UART from being put
> into reset.
> 
> But the objection is valid so I withdraw this patch.

Ok, thanks.

> On MacBooks the host wake pin is ORed with the wake pin of the trackpad
> and keyboard and goes into the SMC.  I think it is only meant to be
> used to wake the system from sleep and not to provide an interrupt
> at runtime.  In theory we could request the SMC's GPE but we couldn't
> tell if the interrupt was sent by the trackpad/keyboard or by the
> Bluetooth controller, or was caused by something else entirely.
> 
> So it's probably better not to use runtime PM at all.  Let me rework
> patch [10/10] and remove this hunk:
> 
> +	/* Macs wire the host wake pin to the System Management Controller,
> +	 * which handles wakeup independently of the operating system.
> +	 */
> +	if (x86_apple_machine) {
> +		err = 0;
> +		goto unlock;
> +	}
> +
> 
> I notice that command 0xfc27 is used to write the sleep_params.
> Does anyone know the command to read them so that we can check the
> defaults Apple is using and find out if they can be optimized?
> 
> 
>> I don't see how runtime pm can work without a host-wake IRQ,
>> so IMHO this commit is wrong. Tip when testing this, make sure
>> that you don't have the bluetooth config panel of e.g. gnome3
>> open as that continuously scans for new devices, so runtime pm
>> never kicks in. I've had a bad configured host IRQ pin on some
>> ASUS laptops using serdev bcm bt and the only way a bt keyboard
>> would work reliable was to keep the config panel open...
> 
> I don't have one of these MacBooks, I'm submitting patches on
> behalf of others,

I didn't realize that, great work on this! It must not be easy
to develop a comprehensive patch set like this without direct hw
access.

> but I'm told that having the gnome panel open
> leads to choppy audio on these machines.  Is that normal?

I've never really tested this.

Regards,

Hans

Re: [PATCH v2 03/10] Bluetooth: hci_bcm: Enable runtime PM despite absence of IRQ

[Marcel Holtmann]

Hi Lukas,

>>> Currently runtime PM is only enabled if a valid host wake IRQ was found.
>>> 
>>> However it is used in ways which seem unrelated to host wake:
>>> E.g., runtime PM is used to force the Bluetooth device on for 5 sec
>>> after a complete packet has been received.
>>> 
>>> Afford this functionality to devices which lack an IRQ by moving the
>>> code to enable runtime PM from bcm_request_irq() to bcm_setup().
>> 
>> Hmm, this seems to not make much sense. If runtime_pm is not enabled
>> (no host wake irq) then the device never runtime suspends, so there
>> is no need to force the device on when we're sending / after a
>> complete packet has been received.
>> 
>> That you give "after a complete packet has been received" as condition
>> example illustrates nicely why I believe this commit is a bad idea,
>> if say a bluetooth keyboard wants to send a keypress HID report after
>> the runtime pm timeout has elapsed, how are we going to receive that
>> report since we're runtime suspended and there is no host wake irq
>> to tell us to wakeup the UART and enable receiving on it again?
> 
> The host UART on MacBooks apparently never runtime suspends, so this
> couldn't have been observed during testing:
> 
> They use either 8250_lpss.c (which has no runtime PM callbacks)
> or 8250_dw.c (which disables the clock during runtime suspend,
> but the MacBooks use a fixed rate clock which can't be disabled).
> Moreover intel_lpss_suspend() excludes the UART from being put
> into reset.
> 
> But the objection is valid so I withdraw this patch.
> 
> On MacBooks the host wake pin is ORed with the wake pin of the trackpad
> and keyboard and goes into the SMC.  I think it is only meant to be
> used to wake the system from sleep and not to provide an interrupt
> at runtime.  In theory we could request the SMC's GPE but we couldn't
> tell if the interrupt was sent by the trackpad/keyboard or by the
> Bluetooth controller, or was caused by something else entirely.
> 
> So it's probably better not to use runtime PM at all.  Let me rework
> patch [10/10] and remove this hunk:
> 
> +	/* Macs wire the host wake pin to the System Management Controller,
> +	 * which handles wakeup independently of the operating system.
> +	 */
> +	if (x86_apple_machine) {
> +		err = 0;
> +		goto unlock;
> +	}
> +
> 
> I notice that command 0xfc27 is used to write the sleep_params.
> Does anyone know the command to read them so that we can check the
> defaults Apple is using and find out if they can be optimized?

the Read Sleepmode Param is 0xfc28 and requires no parameters. You can check monitor/broadcom.c for a bunch of these commands since btmon will decode them for you if possible. Might want to actually post the btmon log of the init sequence here for reference (just blacklist the module and load it manually after starting btmon).

I thought that I created a Broadcom specific init that reads most of the standard values so we have them for reference in the btmon logs. However it seems I have not done that for Sleepmode Param at the moment.

>> I don't see how runtime pm can work without a host-wake IRQ,
>> so IMHO this commit is wrong. Tip when testing this, make sure
>> that you don't have the bluetooth config panel of e.g. gnome3
>> open as that continuously scans for new devices, so runtime pm
>> never kicks in. I've had a bad configured host IRQ pin on some
>> ASUS laptops using serdev bcm bt and the only way a bt keyboard
>> would work reliable was to keep the config panel open...
> 
> I don't have one of these MacBooks, I'm submitting patches on
> behalf of others, but I'm told that having the gnome panel open
> leads to choppy audio on these machines.  Is that normal?
> The Bluetooth controllers are fixed to a baudrate of 3 Mbit
> on these machines.

They need to run btmon and see what is actually going on.

Regards

Marcel

Re: [PATCH v2 10/10] Bluetooth: hci_bcm: Support Apple GPIO handling

[Marcel Holtmann]

Hi Lukas,

> Enable Bluetooth on the following Macs which provide custom ACPI methods
> to toggle the GPIOs for device wake and shutdown instead of accessing
> the pins directly:
> 
>    MacBook8,1     2015  12"
>    MacBook9,1     2016  12"
>    MacBook10,1    2017  12"
>    MacBookPro13,1 2016  13"
>    MacBookPro13,2 2016  13" with Touch Bar
>    MacBookPro13,3 2016  15" with Touch Bar
>    MacBookPro14,1 2017  13"
>    MacBookPro14,2 2017  13" with Touch Bar
>    MacBookPro14,3 2017  15" with Touch Bar
> 
> On the MacBook8,1 Bluetooth is muxed with a second device (a debug port
> on the SSD) under the control of PCH GPIO 36.  Because serdev cannot
> deal with multiple slaves yet, it is currently necessary to patch the
> DSDT and remove the SSDC device.
> 
> The custom ACPI methods are called:
> 
>    BTLP (Low Power) takes one argument, toggles device wake GPIO
>    BTPU (Power Up) tells SMC to drive shutdown GPIO high
>    BTPD (Power Down) tells SMC to drive shutdown GPIO low
>    BTRS (Reset) calls BTPD followed by BTPU
>    BTRB unknown, not present on all MacBooks
> 
> Search for the BTLP, BTPU and BTPD methods on ->probe and cache them in
> struct bcm_device if the machine is a Mac.
> 
> Additionally, set the init_speed based on a custom device property
> provided by Apple in lieu of _CRS resources.  The Broadcom UART's speed
> is fixed on Apple Macs:  Any attempt to change it results in Bluetooth
> status code 0x0c and bcm_set_baudrate() thus always returns -EBUSY.
> By setting only the init_speed and leaving oper_speed at zero, we can
> achieve that the host UART's speed is adjusted but the Broadcom UART's
> speed is left as is.
> 
> The host wake pin goes into the SMC which handles it independently
> of the OS, so there's no IRQ for it.
> 
> Thanks to Ronald Tschalär who did extensive debugging and testing of
> this patch and contributed fixes.
> 
> ACPI snippet containing the custom methods and device properties
> (taken from a MacBook8,1):
> 
>    Method (BTLP, 1, Serialized)
>    {
>        If (LEqual (Arg0, 0x00))
>        {
>            Store (0x01, GD54) /* set PCH GPIO 54 direction to input */
>        }
> 
>        If (LEqual (Arg0, 0x01))
>        {
>            Store (0x00, GD54) /* set PCH GPIO 54 direction to output */
>            Store (0x00, GP54) /* set PCH GPIO 54 value to low */
>        }
>    }
> 
>    Method (BTPU, 0, Serialized)
>    {
>        Store (0x01, \_SB.PCI0.LPCB.EC.BTPC)
>        Sleep (0x0A)
>    }
> 
>    Method (BTPD, 0, Serialized)
>    {
>        Store (0x00, \_SB.PCI0.LPCB.EC.BTPC)
>        Sleep (0x0A)
>    }
> 
>    Method (BTRS, 0, Serialized)
>    {
>        BTPD ()
>        BTPU ()
>    }
> 
>    Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
>    {
>        If (LEqual (Arg0, ToUUID ("a0b5b7c6-1318-441c-b0c9-fe695eaf949b")))
>        {
>            Store (Package (0x08)
>                {
>                    "baud",
>                    Buffer (0x08)
>                    { 0xC0, 0xC6, 0x2D, 0x00, 0x00, 0x00, 0x00, 0x00 },
> 
>                    "parity",
>                    Buffer (0x08)
>                    { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },
> 
>                    "dataBits",
>                    Buffer (0x08)
>                    { 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },
> 
>                    "stopBits",
>                    Buffer (0x08)
>                    { 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 }
>                }, Local0)
>            DTGP (Arg0, Arg1, Arg2, Arg3, RefOf (Local0))
>            Return (Local0)
>        }
>        Return (0x00)
>    }
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=110901
> Reported-by: Leif Liddy <leif.liddy@gmail.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
> Cc: Loic Poulain <loic.poulain@linaro.org>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Max Shavrick <mxms@me.com>                     [MacBook8,1]
> Tested-by: Leif Liddy <leif.liddy@gmail.com>              [MacBook9,1]
> Tested-by: Daniel Roschka <danielroschka@phoenitydawn.de> [MacBookPro13,2]
> Tested-by: Ronald Tschalär <ronald@innovation.ch>         [MacBookPro13,3]
> Tested-by: Peter Y. Chuang <peteryuchuang@gmail.com>      [MacBookPro14,1]
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
> Changes since v1:
>  add DSDT excerpt to the commit message,
>  drop ternary operators for readability,
>  return -EIO instead of -EFAULT if ACPI method calls fail,
>  return -EOPNOTSUPP in inline stubs,
>  use network subsystem comment style. (Marcel, Hans, Andy)
>  Also, to accommodate to mandatory presence of the two GPIOs as per
>  patch [2/10], rename bcm_apple_probe() to bcm_apple_get_resources()
>  and call it from bcm_get_resources() instead of bcm_acpi_probe().
> 
> drivers/bluetooth/hci_bcm.c | 76 +++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 74 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index ad6b7c35eb8e..4e0ba38d39d2 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -29,6 +29,7 @@
> #include <linux/acpi.h>
> #include <linux/of.h>
> #include <linux/property.h>
> +#include <linux/platform_data/x86/apple.h>
> #include <linux/platform_device.h>
> #include <linux/clk.h>
> #include <linux/gpio/consumer.h>
> @@ -75,6 +76,9 @@
>  * @hu: pointer to HCI UART controller struct,
>  *	used to enable flow control during runtime suspend and system sleep
>  * @is_suspended: whether flow control is currently enabled
> + * @btlp: Apple ACPI method to toggle BT_WAKE pin ("BlueTooth Low Power")
> + * @btpu: Apple ACPI method to drive BT_REG_ON pin high ("BlueTooth Power Up")
> + * @btpd: Apple ACPI method to drive BT_REG_ON pin low ("BlueTooth Power Down”)
>  */

unless the Apple ACPI table really use the string “BlueTooth”, I prefer that we use the proper “Bluetooth” string here.

Regards

Marcel

Re: [PATCH v2 02/10] Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO

[Marcel Holtmann]

Hi Lukas,

> Commit 0395ffc1ee05 ("Bluetooth: hci_bcm: Add PM for BCM devices")
> amended this driver to request a shutdown and device wake GPIO on probe,
> but mandated that only one of them need to be present:
> 
> 	/* Make sure at-least one of the GPIO is defined and that
> 	 * a name is specified for this instance
> 	 */
> 	if ((!dev->device_wakeup && !dev->shutdown) || !dev->name) {
> 		dev_err(&pdev->dev, "invalid platform data\n");
> 		return -EINVAL;
> 	}
> 
> However the same commit added a call to bcm_gpio_set_power() to the
> ->probe hook, which unconditionally accesses *both* GPIOs.  Luckily,
> the resulting NULL pointer deref was never reported, suggesting there's
> no machine where either GPIO is missing.
> 
> Commit 8a92056837fd ("Bluetooth: hci_bcm: Add (runtime)pm support to the
> serdev driver") removed the check whether at least one of the GPIOs is
> present without specifying a reason.
> 
> Because commit 62aaefa7d038 ("Bluetooth: hci_bcm: improve use of gpios
> API") refactored the driver to use devm_gpiod_get_optional() instead of
> devm_gpiod_get(), one is now tempted to believe that the driver doesn't
> require *any* of the two GPIOs.
> 
> Which is wrong, the driver still requires both GPIOs to avoid a NULL
> pointer deref.  To this end, establish the status quo ante and request
> the GPIOs with devm_gpiod_get() again.  Bail out of ->probe if either
> of them is missing.
> 
> Oddly enough, whereas bcm_gpio_set_power() accesses the device wake pin
> unconditionally, bcm_suspend_device() and bcm_resume_device() do check
> for its presence before accessing it.  Those checks are superfluous,
> so remove them.
> 
> Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
> Cc: Loic Poulain <loic.poulain@linaro.org>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
> drivers/bluetooth/hci_bcm.c | 24 +++++++++---------------
> 1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 1fc604a0d870..4294d9df1d4d 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -572,11 +572,9 @@ static int bcm_suspend_device(struct device *dev)
> 	}
> 
> 	/* Suspend the device */
> -	if (bdev->device_wakeup) {
> -		gpiod_set_value(bdev->device_wakeup, false);
> -		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
> -		mdelay(15);
> -	}
> +	gpiod_set_value(bdev->device_wakeup, false);
> +	bt_dev_dbg(bdev, "suspend, delaying 15 ms");
> +	mdelay(15);
> 
> 	return 0;
> }
> @@ -587,11 +585,9 @@ static int bcm_resume_device(struct device *dev)
> 
> 	bt_dev_dbg(bdev, "");
> 
> -	if (bdev->device_wakeup) {
> -		gpiod_set_value(bdev->device_wakeup, true);
> -		bt_dev_dbg(bdev, "resume, delaying 15 ms");
> -		mdelay(15);
> -	}
> +	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) {
> @@ -774,14 +770,12 @@ static int bcm_get_resources(struct bcm_device *dev)
> 
> 	dev->clk = devm_clk_get(dev->dev, NULL);
> 
> -	dev->device_wakeup = devm_gpiod_get_optional(dev->dev,
> -						     "device-wakeup",
> -						     GPIOD_OUT_LOW);
> +	dev->device_wakeup = devm_gpiod_get(dev->dev, "device-wakeup",
> +							     GPIOD_OUT_LOW);

I prefer that we keep netdev indentation style.

> 	if (IS_ERR(dev->device_wakeup))
> 		return PTR_ERR(dev->device_wakeup);
> 
> -	dev->shutdown = devm_gpiod_get_optional(dev->dev, "shutdown",
> -						GPIOD_OUT_LOW);
> +	dev->shutdown = devm_gpiod_get(dev->dev, "shutdown", GPIOD_OUT_LOW);
> 	if (IS_ERR(dev->shutdown))
> 		return PTR_ERR(dev->shutdown);

Regards

Marcel

Re: [PATCH v2 05/10] Bluetooth: hci_bcm: Add helper to toggle device wake GPIO

[Andy Shevchenko]

On Tue, 2018-01-02 at 20:08 +0100, Lukas Wunner wrote:
> There is already a bcm_gpio_set_power() helper to toggle the power
> enable pin.  Add a corresponding helper to toggle the device wake pin.
> This will allow us to more easily integrate support for MacBooks which
> use custom ACPI methods to access both pins.
> 
> The only functional change here is an additional 15 ms delay when
> toggling the device wake pin in bcm_gpio_set_power().  It is unclear
> why
> this delay is observed in bcm_suspend_device() and
> bcm_resume_device(),
> but not in bcm_gpio_set_power() so far.

Btw, can we switch to msleep() ?
I don't check if it's atomic context when it's used.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 06/10] Bluetooth: hci_bcm: Silence IRQ printk

[Andy Shevchenko]

On Tue, 2018-01-02 at 20:08 +0100, Lukas Wunner wrote:
> The host wake IRQ is optional, but if none is found, "BCM irq: -22" is
> logged which may irritate users.  This is really a debug message, so
> use
> dev_dbg() instead of dev_info().  If users are interested in the IRQ,
> they can always consult /proc/interrupts.

I object to do in the way how it's done in this patch. See below.

> 
> Cc: Frédéric Danis <frederic.danis.oss@gmail.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/bluetooth/hci_bcm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 01d0943b7bbb..632939d413df 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -798,7 +798,7 @@ static int bcm_get_resources(struct bcm_device
> *dev)
>  		dev->irq = gpiod_to_irq(gpio);
>  	}
>  
> -	dev_info(dev->dev, "BCM irq: %d\n", dev->irq);
> +	dev_dbg(dev->dev, "BCM irq: %d\n", dev->irq);

Perhaps, instead you may add same check

if (dev->irq > 0)
 dev_info(...);
else
 dev_info(..., "BCM irq not in use\n");

?

>  	return 0;
>  }
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 03/10] Bluetooth: hci_bcm: Enable runtime PM despite absence of IRQ

[Lukas Wunner]

On Wed, Jan 03, 2018 at 01:34:00PM +0100, Marcel Holtmann wrote:
> > I notice that command 0xfc27 is used to write the sleep_params.
> > Does anyone know the command to read them so that we can check the
> > defaults Apple is using and find out if they can be optimized?
> 
> the Read Sleepmode Param is 0xfc28 and requires no parameters. You can
> check monitor/broadcom.c for a bunch of these commands since btmon will
> decode them for you if possible. Might want to actually post the btmon
> log of the init sequence here for reference (just blacklist the module
> and load it manually after starting btmon).
> 
> I thought that I created a Broadcom specific init that reads most of
> the standard values so we have them for reference in the btmon logs.
> However it seems I have not done that for Sleepmode Param at the moment.

Thanks!

I've now been able to dig up v1.6 of the Laird SD40 firmware datasheet.

There's an incongruence between the datasheet and struct bcm_set_sleep_mode
(as declared in drivers/bluetooth/btbcm.h and hci_bcm.c) wherein the order
of pulsed_host_wake and break_to_host is reversed.

The datasheet is dated Oct 2015, so fairly recent, making it appear more
likely that the datasheet is correct and our code is wrong?

Kind regards,

Lukas

Re: [PATCH v2 03/10] Bluetooth: hci_bcm: Enable runtime PM despite absence of IRQ

[Marcel Holtmann]

Hi Lukas,

>>> I notice that command 0xfc27 is used to write the sleep_params.
>>> Does anyone know the command to read them so that we can check the
>>> defaults Apple is using and find out if they can be optimized?
>> 
>> the Read Sleepmode Param is 0xfc28 and requires no parameters. You can
>> check monitor/broadcom.c for a bunch of these commands since btmon will
>> decode them for you if possible. Might want to actually post the btmon
>> log of the init sequence here for reference (just blacklist the module
>> and load it manually after starting btmon).
>> 
>> I thought that I created a Broadcom specific init that reads most of
>> the standard values so we have them for reference in the btmon logs.
>> However it seems I have not done that for Sleepmode Param at the moment.
> 
> Thanks!
> 
> I've now been able to dig up v1.6 of the Laird SD40 firmware datasheet.
> 
> There's an incongruence between the datasheet and struct bcm_set_sleep_mode
> (as declared in drivers/bluetooth/btbcm.h and hci_bcm.c) wherein the order
> of pulsed_host_wake and break_to_host is reversed.
> 
> The datasheet is dated Oct 2015, so fairly recent, making it appear more
> likely that the datasheet is correct and our code is wrong?

do you have a link to the data sheet?

Regards

Marcel

Re: [PATCH v2 03/10] Bluetooth: hci_bcm: Enable runtime PM despite absence of IRQ

[Lukas Wunner]

On Wed, Jan 03, 2018 at 09:07:20AM +0100, Hans de Goede wrote:
> On 03-01-18 00:36, Lukas Wunner wrote:
> > I don't have one of these MacBooks, I'm submitting patches on
> > behalf of others,
> 
> I didn't realize that, great work on this! It must not be easy
> to develop a comprehensive patch set like this without direct hw
> access.

A single person wouldn't be able to do it.  It's a collaborative effort,
the testing and debugging on multiple different machines, by Ronald
in particular but also Daniel, Peter, Leif, Max and others, is
indispensable.  (I've acknowledged that in patch [10/10].)

Gathering and curating all the relevant information, as done by Daniel
and Ronald on GitHub, is likewise important, as is the pioneering work
done by Leif.

Thanks,

Lukas

Re: [PATCH v2 03/10] Bluetooth: hci_bcm: Enable runtime PM despite absence of IRQ

[Lukas Wunner]

On Wed, Jan 03, 2018 at 02:29:04PM +0100, Marcel Holtmann wrote:
> do you have a link to the data sheet?

http://docplayer.net/62075870-Bluetooth-functionality-laird-sd40-series-version-1-6.html

Re: [PATCH v2 06/10] Bluetooth: hci_bcm: Silence IRQ printk

[Lukas Wunner]

On Wed, Jan 03, 2018 at 03:08:59PM +0200, Andy Shevchenko wrote:
> On Tue, 2018-01-02 at 20:08 +0100, Lukas Wunner wrote:
> > @@ -798,7 +798,7 @@ static int bcm_get_resources(struct bcm_device
> > *dev)
> >  		dev->irq = gpiod_to_irq(gpio);
> >  	}
> >  
> > -	dev_info(dev->dev, "BCM irq: %d\n", dev->irq);
> > +	dev_dbg(dev->dev, "BCM irq: %d\n", dev->irq);
> 
> Perhaps, instead you may add same check
> 
> if (dev->irq > 0)
>  dev_info(...);
> else
>  dev_info(..., "BCM irq not in use\n");
> 
> ?

gpiod_to_irq() returns different negative errnos depending on the exact
error condition.  We wouldn't be able to easily differentiate them if
we do it this way.

Thanks,

Lukas

Re: [PATCH v2 06/10] Bluetooth: hci_bcm: Silence IRQ printk

[Andy Shevchenko]

On Wed, 2018-01-03 at 14:56 +0100, Lukas Wunner wrote:
> On Wed, Jan 03, 2018 at 03:08:59PM +0200, Andy Shevchenko wrote:
> > On Tue, 2018-01-02 at 20:08 +0100, Lukas Wunner wrote:

> > > -	dev_info(dev->dev, "BCM irq: %d\n", dev->irq);
> > > +	dev_dbg(dev->dev, "BCM irq: %d\n", dev->irq);
> > 
> > Perhaps, instead you may add same check
> > 
> > if (dev->irq > 0)
> >  dev_info(...);
> > else
> >  dev_info(..., "BCM irq not in use\n");

dev_dbg(..., "... (rc: %d)\n", dev->irq);

?

But okay, it's a bikeshedding, when something goes wrong the debug is
the first on the plate to enable anyway.

> gpiod_to_irq() returns different negative errnos depending on the
> exact
> error condition.  We wouldn't be able to easily differentiate them if
> we do it this way.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 09/10] Bluetooth: hci_bcm: Handle errors properly

[Andy Shevchenko]

On Tue, 2018-01-02 at 20:08 +0100, Lukas Wunner wrote:
> A significant portion of this driver lacks error handling.  As a first
> step, add error paths to bcm_gpio_set_power(), bcm_open(),
> bcm_close(),
> bcm_suspend_device(), bcm_resume_device(), bcm_resume(), bcm_probe()
> and
> bcm_serdev_probe().  (I've also scrutinized bcm_suspend() but think
> it's
> fine as is.)
> 
> Those are all the functions accessing the device wake and shutdown
> GPIO.
> On Apple Macs the pins are accessed through ACPI methods, which may
> fail
> for various reasons, hence proper error handling is necessary.  Non-
> Macs
> access the pins directly, which may fail as well but the GPIO core
> does
> not yet pass back errors to consumers.
> 

> +	int err = 0;

Obviously redundant assignment.

> +
> +	if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled) {
> +		err = clk_prepare_enable(dev->clk);
> +		if (err)
> +			return err;
> +	}
>  
>  	gpiod_set_value(dev->shutdown, powered);

> +	err = bcm_gpio_set_device_wakeup(dev, powered);
> +	if (err)
> +		goto err_revert_shutdown;

> +#ifdef CONFIG_PM
> +	bcm->dev->hu = NULL;
> +#endif

Hmm... There is no field in !PM case?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 10/10] Bluetooth: hci_bcm: Support Apple GPIO handling

[Andy Shevchenko]

On Tue, 2018-01-02 at 20:08 +0100, Lukas Wunner wrote:
> Enable Bluetooth on the following Macs which provide custom ACPI
> methods
> to toggle the GPIOs for device wake and shutdown instead of accessing
> the pins directly:

> +static inline int bcm_apple_get_resources(struct bcm_device *dev)
> +{ return -EOPNOTSUPP; }
> +static inline int bcm_apple_set_power(struct bcm_device *dev, bool
> powered)
> +{ return -EOPNOTSUPP; }
> +static inline int bcm_apple_set_low_power(struct bcm_device *dev,
> bool enable)
> +{ return -EOPNOTSUPP; }
> 

Might be a silly question but can we create some struct with callbacks
and assign it differently based on Apple / non-Apple cases?

In that case you don't need conditionals to call (you will always have
them defined to something).

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 09/10] Bluetooth: hci_bcm: Handle errors properly

[Lukas Wunner]

On Wed, Jan 03, 2018 at 06:08:42PM +0200, Andy Shevchenko wrote:
> On Tue, 2018-01-02 at 20:08 +0100, Lukas Wunner wrote:
> > +	int err = 0;
> 
> Obviously redundant assignment.

No, you need to look at it in the context of patch [10/10], which changes
the following portion of bcm_gpio_set_power():

-	gpiod_set_value(dev->shutdown, powered);
+	if (x86_apple_machine)
+		err = bcm_apple_set_power(dev, powered);
+	else
+		gpiod_set_value(dev->shutdown, powered);
+	if (err)
+		goto err_clk_disable;

I need to set err = 0 in case the gpiod_set_value() code path is chosen.

Of course I could only declare "int err;" in this patch and change it to
"int err = 0;" in the next patch, but then I would expect an objection
along the lines of:  You're changing code you've just added in the prior
patch, this is silly.

Long term the gpio API will be changed such that gpiod_set_value()
returns a negative errno or 0 (instead of void), as we're already
doing for gpiod_get_value().  *Then* I can change this to declare
"int err;" and set "err = gpiod_set_value(...)".


> > +#ifdef CONFIG_PM
> > +	bcm->dev->hu = NULL;
> > +#endif
> 
> Hmm... There is no field in !PM case?

Yes, there isn't.

Thanks for the review!

Lukas

Re: [PATCH v2 09/10] Bluetooth: hci_bcm: Handle errors properly

[Andy Shevchenko]

On Wed, 2018-01-03 at 19:54 +0100, Lukas Wunner wrote:
> On Wed, Jan 03, 2018 at 06:08:42PM +0200, Andy Shevchenko wrote:
> > On Tue, 2018-01-02 at 20:08 +0100, Lukas Wunner wrote:
> > > +	int err = 0;
> > 
> > Obviously redundant assignment.
> 
> No, you need to look at it in the context of patch [10/10], which
> changes
> the following portion of bcm_gpio_set_power():

Then why this assignment here?

Move it to related patch.

> 
> -	gpiod_set_value(dev->shutdown, powered);
> +	if (x86_apple_machine)
> +		err = bcm_apple_set_power(dev, powered);
> +	else
> +		gpiod_set_value(dev->shutdown, powered);
> +	if (err)
> +		goto err_clk_disable;
> 
> I need to set err = 0 in case the gpiod_set_value() code path is
> chosen.
> 
> Of course I could only declare "int err;" in this patch and change it
> to
> "int err = 0;" in the next patch, but then I would expect an objection
> along the lines of:  You're changing code you've just added in the
> prior
> patch, this is silly.

Nope. Assignment belongs to when code really needs it. It overrides
ping-ponging style in this case I suppose.

> Long term the gpio API will be changed such that gpiod_set_value()
> returns a negative errno or 0 (instead of void), as we're already
> doing for gpiod_get_value().  *Then* I can change this to declare
> "int err;" and set "err = gpiod_set_value(...)".

It's a good roadmap, though out of scope of the current approach AFAICS.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 01/10] Bluetooth: Depend on rather than select GPIOLIB

[Marcel Holtmann]

Hi Lukas,

> Commit 27378f4c1b92 ("Bluetooth: Avoid WARN splat due to missing
> GPIOLIB") amended Kconfig to select GPIOLIB if BT_HCIUART_NOKIA,
> BT_HCIUART_INTEL or BT_HCIUART_BCM is enabled since all three drivers
> require it to function.
> 
> The diagnosis was correct but the treatment was not.  As stated in
> Documentation/gpio/consumer.txt:
> 
>    Guidelines for GPIOs consumers
>    ==============================
> 
>    Drivers that can't work without standard GPIO calls should have
>    Kconfig entries that depend on GPIOLIB.
>                         ^^^^^^^^^
> Fix it.
> 
> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
> drivers/bluetooth/Kconfig | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel