Enable UART-attached Bluetooth on 2015+ MacBook (Pro), v4.
I forgot to include a changelog in each individual patch in v3,
I'm making up for it now by listing the changes since both, v3 and v2.
Changes since v3:
- In patch [07/13] ("Document struct bcm_device"):
s/enable flow control/disable flow control/ to account for inverse
semantics of hci_uart_set_flow_control().
- In patch [05/13] and [09/13] ("Handle errors properly"):
Fix corner case where an unbalanced pm_runtime_disable() could occur.
- In patch [10/13] ("Support Apple GPIO handling"):
Style fix: Unroll bcm_apple_get_resources() inline stub. (Andy)
Changes since v2:
- In patch [01/13] ("Mandate presence of shutdown and device wake GPIO"):
Keep netdev indentation style. (Marcel)
- New patch [04/13] to fix an IRQ race on ->close.
- New patch [05/13] to fix an unbalanced pm_runtime_disable(). (Andy)
- New patch [06/13] to invalidate the IRQ if requesting it failed.
- New patch [08/13] to add callbacks to toggle GPIOs.
This makes the subsequent addition of Mac support less intrusive.
Replaces patch "Add helper to toggle device wake GPIO" in v2. (Andy)
- In patch [09/13] ("Handle errors properly"):
Drop redundant assignment. (Andy)
- In patch [10/13] ("Support Apple GPIO handling"):
Don't enable runtime PM on Macs for lack of usable host wake IRQ (Hans),
s/BlueTooth/Bluetooth/ in kerneldoc. (Marcel)
- Move patch "Silence IRQ printk" to end of series as it's merely
a cleanup and no longer necessary for Mac support.
- New patch [12/13] to use msleep() instead of mdelay()
after toggling device wake pin. (Andy)
- New patch [13/13] to fix sleep mode struct ordering.
- Drop incorrect patch "Enable runtime PM despite absence of IRQ". (Hans)
Link to v3:
https://www.spinics.net/lists/linux-bluetooth/msg73705.html
Link to v2:
https://www.spinics.net/lists/linux-bluetooth/msg73628.html
Thanks,
Lukas
Lukas Wunner (12):
Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO
Bluetooth: hci_bcm: Clean up unnecessary #ifdef
Bluetooth: hci_bcm: Fix race on close
Bluetooth: hci_bcm: Fix unbalanced pm_runtime_disable()
Bluetooth: hci_bcm: Invalidate IRQ on request failure
Bluetooth: hci_bcm: Document struct bcm_device
Bluetooth: hci_bcm: Add callbacks to toggle GPIOs
Bluetooth: hci_bcm: Handle errors properly
Bluetooth: hci_bcm: Support Apple GPIO handling
Bluetooth: hci_bcm: Silence IRQ printk
Bluetooth: hci_bcm: Sleep instead of spinning
Bluetooth: btbcm: Fix sleep mode struct ordering
Ronald Tschalär (1):
Bluetooth: hci_bcm: Validate IRQ before using it
drivers/bluetooth/btbcm.h | 2 +-
drivers/bluetooth/hci_bcm.c | 228 ++++++++++++++++++++++++++++++++++++--------
2 files changed, 190 insertions(+), 40 deletions(-)
--
2.15.1
Hi Lukas,
> Enable UART-attached Bluetooth on 2015+ MacBook (Pro), v4.
>
> I forgot to include a changelog in each individual patch in v3,
> I'm making up for it now by listing the changes since both, v3 and v2.
>
> Changes since v3:
>
> - In patch [07/13] ("Document struct bcm_device"):
> s/enable flow control/disable flow control/ to account for inverse
> semantics of hci_uart_set_flow_control().
>
> - In patch [05/13] and [09/13] ("Handle errors properly"):
> Fix corner case where an unbalanced pm_runtime_disable() could occur.
>
> - In patch [10/13] ("Support Apple GPIO handling"):
> Style fix: Unroll bcm_apple_get_resources() inline stub. (Andy)
>
> Changes since v2:
>
> - In patch [01/13] ("Mandate presence of shutdown and device wake GPIO"):
> Keep netdev indentation style. (Marcel)
>
> - New patch [04/13] to fix an IRQ race on ->close.
>
> - New patch [05/13] to fix an unbalanced pm_runtime_disable(). (Andy)
>
> - New patch [06/13] to invalidate the IRQ if requesting it failed.
>
> - New patch [08/13] to add callbacks to toggle GPIOs.
> This makes the subsequent addition of Mac support less intrusive.
> Replaces patch "Add helper to toggle device wake GPIO" in v2. (Andy)
>
> - In patch [09/13] ("Handle errors properly"):
> Drop redundant assignment. (Andy)
>
> - In patch [10/13] ("Support Apple GPIO handling"):
> Don't enable runtime PM on Macs for lack of usable host wake IRQ (Hans),
> s/BlueTooth/Bluetooth/ in kerneldoc. (Marcel)
>
> - Move patch "Silence IRQ printk" to end of series as it's merely
> a cleanup and no longer necessary for Mac support.
>
> - New patch [12/13] to use msleep() instead of mdelay()
> after toggling device wake pin. (Andy)
>
> - New patch [13/13] to fix sleep mode struct ordering.
>
> - Drop incorrect patch "Enable runtime PM despite absence of IRQ". (Hans)
>
> Link to v3:
> https://www.spinics.net/lists/linux-bluetooth/msg73705.html
>
> Link to v2:
> https://www.spinics.net/lists/linux-bluetooth/msg73628.html
>
> Thanks,
>
> Lukas
>
>
> Lukas Wunner (12):
> Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO
> Bluetooth: hci_bcm: Clean up unnecessary #ifdef
> Bluetooth: hci_bcm: Fix race on close
> Bluetooth: hci_bcm: Fix unbalanced pm_runtime_disable()
> Bluetooth: hci_bcm: Invalidate IRQ on request failure
> Bluetooth: hci_bcm: Document struct bcm_device
> Bluetooth: hci_bcm: Add callbacks to toggle GPIOs
> Bluetooth: hci_bcm: Handle errors properly
> Bluetooth: hci_bcm: Support Apple GPIO handling
> Bluetooth: hci_bcm: Silence IRQ printk
> Bluetooth: hci_bcm: Sleep instead of spinning
> Bluetooth: btbcm: Fix sleep mode struct ordering
>
> Ronald Tschalär (1):
> Bluetooth: hci_bcm: Validate IRQ before using it
>
> drivers/bluetooth/btbcm.h | 2 +-
> drivers/bluetooth/hci_bcm.c | 228 ++++++++++++++++++++++++++++++++++++--------
> 2 files changed, 190 insertions(+), 40 deletions(-)
all 13 patches have been applied to bluetooth-next tree.
Regards
Marcel
According to the documentation for Laird SD40 radio modules (which use
the BCM4329 chipset), the order of the Enable_BREAK_To_Host and
Pulsed_HOST_WAKE parameters in the sleep mode struct is reversed
vis-à-vis our struct declaration. See page 46 of this PDF:
http://cdn.lairdtech.com/home/brandworld/files/Application%20Note%20-%2040%20Series%20Bluetooth.pdf
The documentation is dated Oct 2015, so fairly recent, making it appear
more likely that the documentation is correct and our code is wrong.
Amend our code to be in congruence with the documentation.
Cc: Sue White <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Lukas Wunner <[email protected]>
---
drivers/bluetooth/btbcm.h | 2 +-
drivers/bluetooth/hci_bcm.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/bluetooth/btbcm.h b/drivers/bluetooth/btbcm.h
index d9e6b41658e5..cfe6ad4cc621 100644
--- a/drivers/bluetooth/btbcm.h
+++ b/drivers/bluetooth/btbcm.h
@@ -44,8 +44,8 @@ struct bcm_set_sleep_mode {
__u8 tristate_control;
__u8 usb_auto_sleep;
__u8 usb_resume_timeout;
- __u8 pulsed_host_wake;
__u8 break_to_host;
+ __u8 pulsed_host_wake;
} __packed;
struct bcm_set_pcm_int_params {
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 461364a00b20..64800cd2796c 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -300,8 +300,8 @@ static const struct bcm_set_sleep_mode default_sleep_params = {
/* Irrelevant USB flags */
.usb_auto_sleep = 0,
.usb_resume_timeout = 0,
+ .break_to_host = 0,
.pulsed_host_wake = 0,
- .break_to_host = 0
};
static int bcm_setup_sleep(struct hci_uart *hu)
--
2.15.1
The driver calls mdelay(15) in the ->suspend, ->resume, ->runtime_suspend
and ->runtime_resume hook, however spinning for such a long period of
time is discouraged as per Documentation/timers/timers-howto.txt.
The use of mdelay() seems unnecessary, it is allowed to sleep in the
system sleep and runtime PM hooks (with the exception of ->suspend_noirq
and ->resume_noirq) and the driver itself also does not rely on a
non-sleeping ->runtime_resume as the only place where a synchronous
resume is performed, in bcm_dequeue(), is called from a work item in
hci_ldisc.c and hci_serdev.c.
So replace the mdelay(15) with msleep(15).
Note that the delay is inserted after asserting or deasserting the
device wake pin, but in bcm_gpio_set_power() that pin is asserted or
deasserted *without* observing a delay. It is thus unclear if the delay
is necessary at all. It is likewise unclear why it is exactly 15 ms,
the commit introducing it, 118612fb9165 ("Bluetooth: hci_bcm: Add
suspend/resume PM functions"), does not provide a rationale.
Cc: Frédéric Danis <[email protected]>
Suggested-and-reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Lukas Wunner <[email protected]>
---
drivers/bluetooth/hci_bcm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 0929a264bffa..461364a00b20 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -656,7 +656,7 @@ static int bcm_suspend_device(struct device *dev)
}
bt_dev_dbg(bdev, "suspend, delaying 15 ms");
- mdelay(15);
+ msleep(15);
return 0;
}
@@ -675,7 +675,7 @@ static int bcm_resume_device(struct device *dev)
}
bt_dev_dbg(bdev, "resume, delaying 15 ms");
- mdelay(15);
+ msleep(15);
/* When this executes, the device has woken up already */
if (bdev->is_suspended && bdev->hu) {
--
2.15.1
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 <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Lukas Wunner <[email protected]>
---
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 3e818a429fc5..0929a264bffa 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -943,7 +943,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
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)
}
Link: https://github.com/Dunedan/mbp-2016-linux/issues/29
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=110901
Reported-by: Leif Liddy <[email protected]>
Cc: Mika Westerberg <[email protected]>
Cc: Frédéric Danis <[email protected]>
Cc: Loic Poulain <[email protected]>
Cc: Hans de Goede <[email protected]>
Tested-by: Max Shavrick <[email protected]> [MacBook8,1]
Tested-by: Leif Liddy <[email protected]> [MacBook9,1]
Tested-by: Daniel Roschka <[email protected]> [MacBookPro13,2]
Tested-by: Ronald Tschalär <[email protected]> [MacBookPro13,3]
Tested-by: Peter Y. Chuang <[email protected]> [MacBookPro14,1]
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Ronald Tschalär <[email protected]>
Signed-off-by: Lukas Wunner <[email protected]>
---
Changes since v3:
Style fix: Unroll bcm_apple_get_resources() inline stub. (Andy)
Changes since v2:
Don't enable runtime PM on Macs for lack of usable host wake IRQ (Hans),
s/BlueTooth/Bluetooth/ in kerneldoc. (Marcel)
drivers/bluetooth/hci_bcm.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 03a365148184..3e818a429fc5 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>
@@ -64,7 +65,12 @@
* @shutdown: BT_REG_ON pin,
* power up or power down Bluetooth device internal regulators
* @set_device_wakeup: callback to toggle BT_WAKE pin
+ * either by accessing @device_wakeup or by calling @btlp
* @set_shutdown: callback to toggle BT_REG_ON pin
+ * either by accessing @shutdown or by calling @btpu/@btpd
+ * @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")
* @clk: clock used by Bluetooth device
* @clk_enabled: whether @clk is prepared and enabled
* @init_speed: default baudrate of Bluetooth device;
@@ -90,6 +96,9 @@ struct bcm_device {
struct gpio_desc *shutdown;
int (*set_device_wakeup)(struct bcm_device *, bool);
int (*set_shutdown)(struct bcm_device *, bool);
+#ifdef CONFIG_ACPI
+ acpi_handle btlp, btpu, btpd;
+#endif
struct clk *clk;
bool clk_enabled;
@@ -844,6 +853,49 @@ static int bcm_resource(struct acpi_resource *ares, void *data)
return 0;
}
+
+static int bcm_apple_set_device_wakeup(struct bcm_device *dev, bool awake)
+{
+ if (ACPI_FAILURE(acpi_execute_simple_method(dev->btlp, NULL, !awake)))
+ return -EIO;
+
+ return 0;
+}
+
+static int bcm_apple_set_shutdown(struct bcm_device *dev, bool powered)
+{
+ if (ACPI_FAILURE(acpi_evaluate_object(powered ? dev->btpu : dev->btpd,
+ NULL, NULL, NULL)))
+ return -EIO;
+
+ return 0;
+}
+
+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;
+
+ dev->set_device_wakeup = bcm_apple_set_device_wakeup;
+ dev->set_shutdown = bcm_apple_set_shutdown;
+
+ return 0;
+}
+#else
+static inline int bcm_apple_get_resources(struct bcm_device *dev)
+{
+ return -EOPNOTSUPP;
+}
#endif /* CONFIG_ACPI */
static int bcm_gpio_set_device_wakeup(struct bcm_device *dev, bool awake)
@@ -862,6 +914,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
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 <[email protected]>
Cc: Hans de Goede <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Lukas Wunner <[email protected]>
---
Changes since v3:
Fix corner case where an unbalanced pm_runtime_disable() could occur.
Changes since v2:
Drop redundant assignment. (Andy)
drivers/bluetooth/hci_bcm.c | 91 +++++++++++++++++++++++++++++++++++++--------
1 file changed, 75 insertions(+), 16 deletions(-)
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 8741e302e6fd..03a365148184 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -197,11 +197,21 @@ static bool bcm_device_exists(struct bcm_device *device)
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;
- dev->set_shutdown(dev, powered);
- dev->set_device_wakeup(dev, powered);
+ if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled) {
+ err = clk_prepare_enable(dev->clk);
+ if (err)
+ return err;
+ }
+
+ err = dev->set_shutdown(dev, powered);
+ if (err)
+ goto err_clk_disable;
+
+ err = dev->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);
@@ -209,6 +219,13 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
dev->clk_enabled = powered;
return 0;
+
+err_revert_shutdown:
+ dev->set_shutdown(dev, !powered);
+err_clk_disable:
+ if (powered && !IS_ERR(dev->clk) && !dev->clk_enabled)
+ clk_disable_unprepare(dev->clk);
+ return err;
}
#ifdef CONFIG_PM
@@ -331,6 +348,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 +363,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 +394,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);
@@ -407,8 +441,11 @@ static int bcm_close(struct hci_uart *hu)
pm_runtime_disable(bdev->dev);
}
- bcm_gpio_set_power(bdev, false);
- 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_set_suspended(bdev->dev);
}
mutex_unlock(&bcm_device_lock);
@@ -588,6 +625,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, "");
@@ -599,7 +637,15 @@ static int bcm_suspend_device(struct device *dev)
}
/* Suspend the device */
- bdev->set_device_wakeup(bdev, false);
+ err = bdev->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;
+ }
+
bt_dev_dbg(bdev, "suspend, delaying 15 ms");
mdelay(15);
@@ -609,10 +655,16 @@ 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, "");
- bdev->set_device_wakeup(bdev, true);
+ err = bdev->set_device_wakeup(bdev, true);
+ if (err) {
+ dev_err(dev, "Failed to power up\n");
+ return err;
+ }
+
bt_dev_dbg(bdev, "resume, delaying 15 ms");
mdelay(15);
@@ -666,6 +718,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);
@@ -685,14 +738,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;
}
@@ -923,7 +978,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;
}
@@ -1025,7 +1082,9 @@ static int bcm_serdev_probe(struct serdev_device *serdev)
if (err)
return err;
- bcm_gpio_set_power(bcmdev, false);
+ 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
MacBooks provides custom ACPI methods to toggle the GPIOs for device
wake and shutdown instead of accessing the pins directly. Prepare for
their support by adding callbacks to toggle the GPIOs, which on non-Macs
do nothing more but call gpiod_set_value().
No functional change intended.
Suggested-and-reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Lukas Wunner <[email protected]>
---
Changes since v2:
Replaces patch "Add helper to toggle device wake GPIO" in v2.
This makes the subsequent addition of Mac support less intrusive. (Andy)
drivers/bluetooth/hci_bcm.c | 27 +++++++++++++++++++++++----
1 file changed, 23 insertions(+), 4 deletions(-)
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 3c1282a9fcd4..8741e302e6fd 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -63,6 +63,8 @@
* deassert = Bluetooth device may sleep when sleep criteria are met
* @shutdown: BT_REG_ON pin,
* power up or power down Bluetooth device internal regulators
+ * @set_device_wakeup: callback to toggle BT_WAKE pin
+ * @set_shutdown: callback to toggle BT_REG_ON pin
* @clk: clock used by Bluetooth device
* @clk_enabled: whether @clk is prepared and enabled
* @init_speed: default baudrate of Bluetooth device;
@@ -86,6 +88,8 @@ struct bcm_device {
const char *name;
struct gpio_desc *device_wakeup;
struct gpio_desc *shutdown;
+ int (*set_device_wakeup)(struct bcm_device *, bool);
+ int (*set_shutdown)(struct bcm_device *, bool);
struct clk *clk;
bool clk_enabled;
@@ -196,8 +200,8 @@ 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);
+ dev->set_shutdown(dev, powered);
+ dev->set_device_wakeup(dev, powered);
if (!powered && !IS_ERR(dev->clk) && dev->clk_enabled)
clk_disable_unprepare(dev->clk);
@@ -595,7 +599,7 @@ static int bcm_suspend_device(struct device *dev)
}
/* Suspend the device */
- gpiod_set_value(bdev->device_wakeup, false);
+ bdev->set_device_wakeup(bdev, false);
bt_dev_dbg(bdev, "suspend, delaying 15 ms");
mdelay(15);
@@ -608,7 +612,7 @@ static int bcm_resume_device(struct device *dev)
bt_dev_dbg(bdev, "");
- gpiod_set_value(bdev->device_wakeup, true);
+ bdev->set_device_wakeup(bdev, true);
bt_dev_dbg(bdev, "resume, delaying 15 ms");
mdelay(15);
@@ -787,6 +791,18 @@ static int bcm_resource(struct acpi_resource *ares, void *data)
}
#endif /* CONFIG_ACPI */
+static int bcm_gpio_set_device_wakeup(struct bcm_device *dev, bool awake)
+{
+ gpiod_set_value(dev->device_wakeup, awake);
+ return 0;
+}
+
+static int bcm_gpio_set_shutdown(struct bcm_device *dev, bool powered)
+{
+ gpiod_set_value(dev->shutdown, powered);
+ return 0;
+}
+
static int bcm_get_resources(struct bcm_device *dev)
{
dev->name = dev_name(dev->dev);
@@ -802,6 +818,9 @@ static int bcm_get_resources(struct bcm_device *dev)
if (IS_ERR(dev->shutdown))
return PTR_ERR(dev->shutdown);
+ dev->set_device_wakeup = bcm_gpio_set_device_wakeup;
+ dev->set_shutdown = bcm_gpio_set_shutdown;
+
/* IRQ can be declared in ACPI table as Interrupt or GpioInt */
if (dev->irq <= 0) {
struct gpio_desc *gpio;
--
2.15.1
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Lukas Wunner <[email protected]>
---
Changes since v3:
s/enable flow control/disable flow control/ to account for inverse
semantics of hci_uart_set_flow_control().
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 c893c597fead..3c1282a9fcd4 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 disable flow control during runtime suspend and system sleep
+ * @is_suspended: whether flow control is currently disabled
+ */
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
If devm_request_irq() fails, the driver bails out of bcm_request_irq()
but continues to ->setup the device (because the IRQ is optional).
The driver subsequently calls devm_free_irq(), enable_irq_wake() and
disable_irq_wake() on the IRQ even though requesting it failed.
Avoid by invalidating the IRQ on request failure.
Cc: Frédéric Danis <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Lukas Wunner <[email protected]>
---
drivers/bluetooth/hci_bcm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 140c7e5bf812..c893c597fead 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -216,8 +216,10 @@ static int bcm_request_irq(struct bcm_data *bcm)
bdev->irq_active_low ? IRQF_TRIGGER_FALLING :
IRQF_TRIGGER_RISING,
"host_wake", bdev);
- if (err)
+ if (err) {
+ bdev->irq = err;
goto unlock;
+ }
device_init_wakeup(bdev->dev, true);
--
2.15.1
On ->setup, pm_runtime_enable() is only called if a valid IRQ was found,
but on ->close(), pm_runtime_disable() is called unconditionally.
Disablement of runtime PM is recorded in a counter, so every
pm_runtime_disable() needs to be balanced. Fix it.
Cc: Frédéric Danis <[email protected]>
Reported-and-reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Lukas Wunner <[email protected]>
---
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 6144a3f9c37a..140c7e5bf812 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -375,10 +375,10 @@ static int bcm_close(struct hci_uart *hu)
if (IS_ENABLED(CONFIG_PM) && bdev->irq > 0) {
devm_free_irq(bdev->dev, bdev->irq, bdev);
device_init_wakeup(bdev->dev, false);
+ pm_runtime_disable(bdev->dev);
}
bcm_gpio_set_power(bdev, false);
- pm_runtime_disable(bdev->dev);
pm_runtime_set_suspended(bdev->dev);
}
mutex_unlock(&bcm_device_lock);
--
2.15.1
Upon ->close, the driver powers the Bluetooth controller down, deasserts
the device wake pin, updates the runtime PM status to "suspended" and
finally frees the IRQ.
Because the IRQ is freed last, a runtime resume can take place after
the controller was powered down. The impact is not grave, the worst
thing that can happen is that the device wake pin is reasserted (should
have no effect while the regulator is off) and that setting the runtime
PM status to "suspended" does not reflect reality.
Still, it's wrong, so free the IRQ first.
Cc: Frédéric Danis <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Lukas Wunner <[email protected]>
---
drivers/bluetooth/hci_bcm.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index c551ef4c350f..6144a3f9c37a 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -372,14 +372,14 @@ 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);
-
if (IS_ENABLED(CONFIG_PM) && bdev->irq > 0) {
devm_free_irq(bdev->dev, bdev->irq, bdev);
device_init_wakeup(bdev->dev, false);
}
+
+ bcm_gpio_set_power(bdev, false);
+ pm_runtime_disable(bdev->dev);
+ pm_runtime_set_suspended(bdev->dev);
}
mutex_unlock(&bcm_device_lock);
--
2.15.1
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 <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Lukas Wunner <[email protected]>
---
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 d9f71ed26667..c551ef4c350f 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -373,15 +373,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
From: Ronald Tschalär <[email protected]>
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]
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Ronald Tschalär <[email protected]>
[lukas: fix up ->suspend and ->resume as well, add commit message]
Signed-off-by: Lukas Wunner <[email protected]>
---
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 7d059b35c63a..d9f71ed26667 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -377,7 +377,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);
}
@@ -623,7 +623,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");
@@ -653,7 +653,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
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 <[email protected]>
Cc: Loic Poulain <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Uwe Kleine-König <[email protected]>
Cc: Linus Walleij <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Lukas Wunner <[email protected]>
---
Changes since v2:
Keep netdev indentation style. (Marcel)
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..7d059b35c63a 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