From 0a03f98b98c201191e3ba15a0e33f46d8660e1fd Mon Sep 17 00:00:00 2001 From: Ioan Moldovan Date: Mon, 28 Aug 2017 18:09:39 +0300 Subject: [PATCH 01/23] Bluetooth: Add a new 04ca:3015 QCA_ROME device This patch adds the 04ca:3015 (from a QCA9377 board) Bluetooth device to the btusb blacklist and makes the kernel use the btqca module instead of btusb. The patch is necessary because, without it the 04ca:3015 device defaults to using the btusb driver, which makes the WIFI side of the QCA9377 board unusable (obtains 0 MBps in speedtest, when the 04ca:3015 bluetooth is used with an audio headset). /sys/kernel/debug/usb/devices: T: Bus=01 Lev=01 Prnt=01 Port=04 Cnt=01 Dev#= 2 Spd=12 MxCh= 0 D: Ver= 2.01 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs= 1 P: Vendor=04ca ProdID=3015 Rev= 0.01 C:* #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA I:* If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb E: Ad=81(I) Atr=03(Int.) MxPS= 16 Ivl=1ms E: Ad=82(I) Atr=02(Bulk) MxPS= 64 Ivl=0ms E: Ad=02(O) Atr=02(Bulk) MxPS= 64 Ivl=0ms I:* If#= 1 Alt= 0 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb E: Ad=83(I) Atr=01(Isoc) MxPS= 0 Ivl=1ms E: Ad=03(O) Atr=01(Isoc) MxPS= 0 Ivl=1ms I: If#= 1 Alt= 1 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb E: Ad=83(I) Atr=01(Isoc) MxPS= 9 Ivl=1ms E: Ad=03(O) Atr=01(Isoc) MxPS= 9 Ivl=1ms I: If#= 1 Alt= 2 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb E: Ad=83(I) Atr=01(Isoc) MxPS= 17 Ivl=1ms E: Ad=03(O) Atr=01(Isoc) MxPS= 17 Ivl=1ms I: If#= 1 Alt= 3 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb E: Ad=83(I) Atr=01(Isoc) MxPS= 25 Ivl=1ms E: Ad=03(O) Atr=01(Isoc) MxPS= 25 Ivl=1ms I: If#= 1 Alt= 4 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb E: Ad=83(I) Atr=01(Isoc) MxPS= 33 Ivl=1ms E: Ad=03(O) Atr=01(Isoc) MxPS= 33 Ivl=1ms I: If#= 1 Alt= 5 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb E: Ad=83(I) Atr=01(Isoc) MxPS= 49 Ivl=1ms E: Ad=03(O) Atr=01(Isoc) MxPS= 49 Ivl=1ms Signed-off-by: Ioan Moldovan Signed-off-by: Johan Hedberg --- drivers/bluetooth/btusb.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index 808c249845db..d1512fbf820d 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -271,6 +271,7 @@ static const struct usb_device_id blacklist_table[] = { { USB_DEVICE(0x0489, 0xe09f), .driver_info = BTUSB_QCA_ROME }, { USB_DEVICE(0x0489, 0xe0a2), .driver_info = BTUSB_QCA_ROME }, { USB_DEVICE(0x04ca, 0x3011), .driver_info = BTUSB_QCA_ROME }, + { USB_DEVICE(0x04ca, 0x3015), .driver_info = BTUSB_QCA_ROME }, { USB_DEVICE(0x04ca, 0x3016), .driver_info = BTUSB_QCA_ROME }, /* Broadcom BCM2035 */ From 27378f4c1b92fe275236c69d2f7f2c27090e7b53 Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Tue, 26 Dec 2017 17:07:34 +0200 Subject: [PATCH 02/23] Bluetooth: Avoid WARN splat due to missing GPIOLIB Loading hci_bcm with CONFIG_GPIOLIB=n results in the following splat when calling gpiod_to_irq() from bcm_get_resources(): WARNING: CPU: 0 PID: 1006 at ./include/linux/gpio/consumer.h:450 bcm_get_resources+0x50/0x80 CPU: 0 PID: 1006 Comm: kworker/u8:4 Tainted: G A 4.15.0-rc4custom+ #4 Hardware name: Apple Inc. MacBook8,1/Mac-BE0E8AC46FE800CC, BIOS MB81.88Z.0168.B00.1708080033 08/08/2017 Call Trace: bcm_serdev_probe+0x8b/0xc0 driver_probe_device+0x202/0x310 __driver_attach+0x85/0x90 ? driver_probe_device+0x310/0x310 bus_for_each_dev+0x57/0x80 async_run_entry_fn+0x2c/0xd0 process_one_work+0x1d2/0x3d0 worker_thread+0x26/0x3c0 ? process_one_work+0x3d0/0x3d0 kthread+0x10c/0x130 ? kthread_create_on_node+0x40/0x40 ret_from_fork+0x1f/0x30 We could call gpiod_to_irq() only if IS_ENABLED(CONFIG_GPIOLIB) but without GPIOLIB, the driver's power saving features can't be used, so selecting GPIOLIB seems more appropriate. The same issue is present in hci_intel.c and hci_nokia.c, fix those up as well. Reported-by: Max Shavrick Signed-off-by: Lukas Wunner Signed-off-by: Marcel Holtmann --- drivers/bluetooth/Kconfig | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig index 45a2f59cd935..41932f0e68d0 100644 --- a/drivers/bluetooth/Kconfig +++ b/drivers/bluetooth/Kconfig @@ -110,6 +110,7 @@ config BT_HCIUART_NOKIA 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,6 +171,7 @@ config BT_HCIUART_INTEL depends on BT_HCIUART 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. @@ -183,6 +185,7 @@ config BT_HCIUART_BCM depends on (!ACPI || SERIAL_DEV_CTRL_TTYPORT) 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. From 43fff768346810042836df325d736bd2c2a634a7 Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Tue, 26 Dec 2017 17:07:34 +0200 Subject: [PATCH 03/23] Bluetooth: hci_bcm: Streamline runtime PM code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This driver seeks to force the Bluetooth device on for the duration of 5 seconds when the Bluetooth device has woken the host and after a complete packet has been received. It does that by calling: pm_runtime_get(); pm_runtime_mark_last_busy(); pm_runtime_put_autosuspend(); The same can be achieved more succinctly with: pm_request_resume(); That's because after runtime resuming the device, rpm_resume() invokes pm_runtime_mark_last_busy() followed by rpm_idle(), which will cause the device to be suspended after expiration of the autosuspend_delay. No functional change intended. Cc: Frédéric Danis Signed-off-by: Lukas Wunner Signed-off-by: Marcel Holtmann --- drivers/bluetooth/hci_bcm.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c index 47fc58c9eb49..1fc604a0d870 100644 --- a/drivers/bluetooth/hci_bcm.c +++ b/drivers/bluetooth/hci_bcm.c @@ -191,9 +191,7 @@ static irqreturn_t bcm_host_wake(int irq, void *data) bt_dev_dbg(bdev, "Host wake IRQ"); - pm_runtime_get(bdev->dev); - pm_runtime_mark_last_busy(bdev->dev); - pm_runtime_put_autosuspend(bdev->dev); + pm_request_resume(bdev->dev); return IRQ_HANDLED; } @@ -512,11 +510,8 @@ static int bcm_recv(struct hci_uart *hu, const void *data, int count) } else if (!bcm->rx_skb) { /* Delay auto-suspend when receiving completed packet */ mutex_lock(&bcm_device_lock); - if (bcm->dev && bcm_device_exists(bcm->dev)) { - pm_runtime_get(bcm->dev->dev); - pm_runtime_mark_last_busy(bcm->dev->dev); - pm_runtime_put_autosuspend(bcm->dev->dev); - } + if (bcm->dev && bcm_device_exists(bcm->dev)) + pm_request_resume(bcm->dev->dev); mutex_unlock(&bcm_device_lock); } From 7d06d5895c159f64c46560dc258e553ad8670fe0 Mon Sep 17 00:00:00 2001 From: Kai-Heng Feng Date: Wed, 20 Dec 2017 19:00:07 +0800 Subject: [PATCH 04/23] Revert "Bluetooth: btusb: fix QCA Rome suspend/resume" This reverts commit fd865802c66bc451dc515ed89360f84376ce1a56. This commit causes a regression on some QCA ROME chips. The USB device reset happens in btusb_open(), hence firmware loading gets interrupted. Furthermore, this commit stops working after commit ("a0085f2510e8976614ad8f766b209448b385492f Bluetooth: btusb: driver to enable the usb-wakeup feature"). Reset-resume quirk only gets enabled in btusb_suspend() when it's not a wakeup source. If we really want to reset the USB device, we need to do it before btusb_open(). Let's handle it in drivers/usb/core/quirks.c. Cc: stable@vger.kernel.org Cc: Leif Liddy Cc: Matthias Kaehlcke Cc: Brian Norris Cc: Daniel Drake Signed-off-by: Kai-Heng Feng Reviewed-by: Brian Norris Tested-by: Brian Norris Signed-off-by: Marcel Holtmann --- drivers/bluetooth/btusb.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index d1512fbf820d..d6fcb4d31548 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -3119,12 +3119,6 @@ static int btusb_probe(struct usb_interface *intf, if (id->driver_info & BTUSB_QCA_ROME) { data->setup_on_usb = btusb_setup_qca; hdev->set_bdaddr = btusb_set_bdaddr_ath3012; - - /* QCA Rome devices lose their updated firmware over suspend, - * but the USB hub doesn't notice any status change. - * Explicitly request a device reset on resume. - */ - set_bit(BTUSB_RESET_RESUME, &data->flags); } #ifdef CONFIG_BT_HCIBTUSB_RTL From e5a49ee98191eedf208ea169455667deb80fe646 Mon Sep 17 00:00:00 2001 From: AceLan Kao Date: Tue, 2 Jan 2018 10:41:11 +0800 Subject: [PATCH 05/23] Bluetooth: btusb: Add support for 0cf3:e010 Device 0cf3:e010 is one of the QCA ROME family. T: Bus=01 Lev=01 Prnt=01 Port=13 Cnt=03 Dev#= 4 Spd=12 MxCh= 0 D: Ver= 2.01 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs= 1 P: Vendor=0cf3 ProdID=e010 Rev=00.01 C: #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA I: If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb I: If#= 1 Alt= 0 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb Signed-off-by: AceLan Kao Signed-off-by: Marcel Holtmann --- drivers/bluetooth/btusb.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index d6fcb4d31548..dcd2f9861d8e 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -264,6 +264,7 @@ static const struct usb_device_id blacklist_table[] = { /* QCA ROME chipset */ { USB_DEVICE(0x0cf3, 0xe007), .driver_info = BTUSB_QCA_ROME }, { USB_DEVICE(0x0cf3, 0xe009), .driver_info = BTUSB_QCA_ROME }, + { USB_DEVICE(0x0cf3, 0xe010), .driver_info = BTUSB_QCA_ROME }, { USB_DEVICE(0x0cf3, 0xe300), .driver_info = BTUSB_QCA_ROME }, { USB_DEVICE(0x0cf3, 0xe301), .driver_info = BTUSB_QCA_ROME }, { USB_DEVICE(0x0cf3, 0xe360), .driver_info = BTUSB_QCA_ROME }, From b71b25fed1c3fa7fdeba4bbda2d17fb384a80508 Mon Sep 17 00:00:00 2001 From: Arnd Bergmann Date: Tue, 2 Jan 2018 11:50:30 +0100 Subject: [PATCH 06/23] Bluetooth: hciuart: add nvmem dependency When the hci support is built-in, but mvmem is a loadable module, we get a link failure: drivers/bluetooth/hci_ll.o: In function `hci_ti_probe': hci_ll.c:(.text+0x226): undefined reference to `nvmem_cell_get' hci_ll.c:(.text+0x238): undefined reference to `nvmem_cell_read' hci_ll.c:(.text+0x244): undefined reference to `nvmem_cell_put' This adds another Kconfig dependency to enforce valid configurations. Fixes: 0e58d0cdb3eb ("Bluetooth: hci_ll: Add optional nvmem BD address source") Signed-off-by: Arnd Bergmann Signed-off-by: Marcel Holtmann --- drivers/bluetooth/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig index 41932f0e68d0..d816ce9e23a7 100644 --- a/drivers/bluetooth/Kconfig +++ b/drivers/bluetooth/Kconfig @@ -77,6 +77,7 @@ config BT_HCIBTSDIO config BT_HCIUART tristate "HCI UART driver" depends on SERIAL_DEV_BUS || !SERIAL_DEV_BUS + depends on NVMEM || !NVMEM depends on TTY help Bluetooth HCI UART driver. From a3a446c7c0e643f740990a7bc2950e75856beb3c Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Tue, 2 Jan 2018 20:08:40 +0100 Subject: [PATCH 07/23] Bluetooth: Depend on rather than select GPIOLIB 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 Signed-off-by: Lukas Wunner Reviewed-by: Linus Walleij Signed-off-by: Marcel Holtmann --- 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. From cca32837f0f5fb794d0d486bc76ed0bfb703de7b Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Sat, 6 Jan 2018 16:15:14 +0000 Subject: [PATCH 08/23] Bluetooth: bpa10x: make array 'req' static, shrinks object size Don't populate the const read-only array 'req' on the stack but instead make it static. Makes the object code smaller by over 40 bytes: Before: text data bss dec hex filename 8497 3408 128 12033 2f01 linux/drivers/bluetooth/bpa10x.o After: text data bss dec hex filename 8366 3496 128 11990 2ed6 linux/drivers/bluetooth/bpa10x.o (gcc version 7.2.0 x86_64) Signed-off-by: Colin Ian King Signed-off-by: Marcel Holtmann --- drivers/bluetooth/bpa10x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/bluetooth/bpa10x.c b/drivers/bluetooth/bpa10x.c index 7971bfbd4321..801ea4ca65e4 100644 --- a/drivers/bluetooth/bpa10x.c +++ b/drivers/bluetooth/bpa10x.c @@ -259,7 +259,7 @@ static int bpa10x_flush(struct hci_dev *hdev) static int bpa10x_setup(struct hci_dev *hdev) { - const u8 req[] = { 0x07 }; + static const u8 req[] = { 0x07 }; struct sk_buff *skb; BT_DBG("%s", hdev->name); From 948c7ca03c14d9dd5ece5866dc1ab49f95613ad6 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Sat, 6 Jan 2018 16:23:28 +0000 Subject: [PATCH 09/23] Bluetooth: btintel: make array 'param' static, shrinks object size Don't populate the const read-only array 'param' on the stack but instead make it static. Makes the object code smaller by nearly 20 bytes: Before: text data bss dec hex filename 11605 2629 64 14298 37da linux/drivers/bluetooth/btintel.o After: text data bss dec hex filename 11531 2685 64 14280 37c8 linux/drivers/bluetooth/btintel.o (gcc version 7.2.0 x86_64) Signed-off-by: Colin Ian King Signed-off-by: Marcel Holtmann --- drivers/bluetooth/btintel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c index 4459555c9d88..07f00e422e85 100644 --- a/drivers/bluetooth/btintel.c +++ b/drivers/bluetooth/btintel.c @@ -75,7 +75,7 @@ EXPORT_SYMBOL_GPL(btintel_check_bdaddr); int btintel_enter_mfg(struct hci_dev *hdev) { - const u8 param[] = { 0x01, 0x00 }; + static const u8 param[] = { 0x01, 0x00 }; struct sk_buff *skb; skb = __hci_cmd_sync(hdev, 0xfc11, 2, param, HCI_CMD_TIMEOUT); From 61f5acea8737d9b717fcc22bb6679924f3c82b98 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Mon, 8 Jan 2018 10:44:16 +0100 Subject: [PATCH 10/23] Bluetooth: btusb: Restore QCA Rome suspend/resume fix with a "rewritten" version Commit 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") removed the setting of the BTUSB_RESET_RESUME quirk for QCA Rome devices, instead favoring adding USB_QUIRK_RESET_RESUME quirks in usb/core/quirks.c. This was done because the DIY BTUSB_RESET_RESUME reset-resume handling has several issues (see the original commit message). An added advantage of moving over to the USB-core reset-resume handling is that it also disables autosuspend for these devices, which is similarly broken on these. But there are 2 issues with this approach: 1) It leaves the broken DIY BTUSB_RESET_RESUME code in place for Realtek devices. 2) Sofar only 2 of the 10 QCA devices known to the btusb code have been added to usb/core/quirks.c and if we fix the Realtek case the same way we need to add an additional 14 entries. So in essence we need to duplicate a large part of the usb_device_id table in btusb.c in usb/core/quirks.c and manually keep them in sync. This commit instead restores setting a reset-resume quirk for QCA devices in the btusb.c code, avoiding the duplicate usb_device_id table problem. This commit avoids the problems with the original DIY BTUSB_RESET_RESUME code by simply setting the USB_QUIRK_RESET_RESUME quirk directly on the usb_device. This commit also moves the BTUSB_REALTEK case over to directly setting the USB_QUIRK_RESET_RESUME on the usb_device and removes the now unused BTUSB_RESET_RESUME code. BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1514836 Fixes: 7d06d5895c15 ("Revert "Bluetooth: btusb: fix QCA...suspend/resume"") Cc: stable@vger.kernel.org Cc: Leif Liddy Cc: Matthias Kaehlcke Cc: Brian Norris Cc: Daniel Drake Cc: Kai-Heng Feng Signed-off-by: Hans de Goede Signed-off-by: Marcel Holtmann --- drivers/bluetooth/btusb.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index dcd2f9861d8e..29977ebfd031 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -390,9 +391,8 @@ static const struct usb_device_id blacklist_table[] = { #define BTUSB_FIRMWARE_LOADED 7 #define BTUSB_FIRMWARE_FAILED 8 #define BTUSB_BOOTING 9 -#define BTUSB_RESET_RESUME 10 -#define BTUSB_DIAG_RUNNING 11 -#define BTUSB_OOB_WAKE_ENABLED 12 +#define BTUSB_DIAG_RUNNING 10 +#define BTUSB_OOB_WAKE_ENABLED 11 struct btusb_data { struct hci_dev *hdev; @@ -3120,6 +3120,12 @@ static int btusb_probe(struct usb_interface *intf, if (id->driver_info & BTUSB_QCA_ROME) { data->setup_on_usb = btusb_setup_qca; hdev->set_bdaddr = btusb_set_bdaddr_ath3012; + + /* QCA Rome devices lose their updated firmware over suspend, + * but the USB hub doesn't notice any status change. + * explicitly request a device reset on resume. + */ + interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME; } #ifdef CONFIG_BT_HCIBTUSB_RTL @@ -3130,7 +3136,7 @@ static int btusb_probe(struct usb_interface *intf, * but the USB hub doesn't notice any status change. * Explicitly request a device reset on resume. */ - set_bit(BTUSB_RESET_RESUME, &data->flags); + interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME; } #endif @@ -3299,14 +3305,6 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message) enable_irq(data->oob_wake_irq); } - /* Optionally request a device reset on resume, but only when - * wakeups are disabled. If wakeups are enabled we assume the - * device will stay powered up throughout suspend. - */ - if (test_bit(BTUSB_RESET_RESUME, &data->flags) && - !device_may_wakeup(&data->udev->dev)) - data->udev->reset_resume = 1; - return 0; } From 3e81a4ca51a1172253078ca7abd6a91040b8fcf4 Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Wed, 10 Jan 2018 16:32:10 +0100 Subject: [PATCH 11/23] Bluetooth: hci_bcm: Mandate presence of shutdown and device wake GPIO MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Cc: Loic Poulain Cc: Hans de Goede Cc: Uwe Kleine-König Cc: Linus Walleij Reviewed-by: Andy Shevchenko Signed-off-by: Lukas Wunner Signed-off-by: Marcel Holtmann --- 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); From 4a59f1fab91e9445e34c69d8e4401a0d6bdbe914 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ronald=20Tschal=C3=A4r?= Date: Wed, 10 Jan 2018 16:32:10 +0100 Subject: [PATCH 12/23] Bluetooth: hci_bcm: Validate IRQ before using it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Ronald Tschalär [lukas: fix up ->suspend and ->resume as well, add commit message] Signed-off-by: Lukas Wunner Signed-off-by: Marcel Holtmann --- 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"); } From 6d83f1ee88a47d81f29d3aa4d17ad68d73473d40 Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Wed, 10 Jan 2018 16:32:10 +0100 Subject: [PATCH 13/23] Bluetooth: hci_bcm: Clean up unnecessary #ifdef MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Andy Shevchenko Signed-off-by: Lukas Wunner Signed-off-by: Marcel Holtmann --- 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); From 54ba69f9e7304e15e07713b4bb0eb1a405d271f0 Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Wed, 10 Jan 2018 16:32:10 +0100 Subject: [PATCH 14/23] Bluetooth: hci_bcm: Fix race on close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Andy Shevchenko Signed-off-by: Lukas Wunner Signed-off-by: Marcel Holtmann --- 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); From f4cf6b7e3b8704d3dabfb8427a61d4c03ce48d80 Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Wed, 10 Jan 2018 16:32:10 +0100 Subject: [PATCH 15/23] Bluetooth: hci_bcm: Fix unbalanced pm_runtime_disable() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reported-and-reviewed-by: Andy Shevchenko Signed-off-by: Lukas Wunner Signed-off-by: Marcel Holtmann --- 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); From 4dc273306c16b04f696e8ab0e8bb4f6e238fe61b Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Wed, 10 Jan 2018 16:32:10 +0100 Subject: [PATCH 16/23] Bluetooth: hci_bcm: Invalidate IRQ on request failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Andy Shevchenko Signed-off-by: Lukas Wunner Signed-off-by: Marcel Holtmann --- 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); From b7c2abac14e1fdf6136a8b54c6151ffe300b961b Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Wed, 10 Jan 2018 16:32:10 +0100 Subject: [PATCH 17/23] Bluetooth: hci_bcm: Document struct bcm_device Reviewed-by: Andy Shevchenko Signed-off-by: Lukas Wunner Signed-off-by: Marcel Holtmann --- 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 }; From 8353b4a636f2c83de748656acc6b6a8757d7a0ff Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Wed, 10 Jan 2018 16:32:10 +0100 Subject: [PATCH 18/23] Bluetooth: hci_bcm: Add callbacks to toggle GPIOs 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 Signed-off-by: Lukas Wunner Signed-off-by: Marcel Holtmann --- 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; From 8bfa7e1e03aca3626b82857850a1e18ae0ed291d Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Wed, 10 Jan 2018 16:32:10 +0100 Subject: [PATCH 19/23] Bluetooth: hci_bcm: Handle errors properly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Cc: Hans de Goede Reviewed-by: Andy Shevchenko Signed-off-by: Lukas Wunner Signed-off-by: Marcel Holtmann --- 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 @@ out: 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 @@ unlock: 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); } From 4c33162c1ad0d3524455d6c10de2a05847ad5617 Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Wed, 10 Jan 2018 16:32:10 +0100 Subject: [PATCH 20/23] Bluetooth: hci_bcm: Support Apple GPIO handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Cc: Mika Westerberg Cc: Frédéric Danis Cc: Loic Poulain Cc: Hans de Goede Tested-by: Max Shavrick [MacBook8,1] Tested-by: Leif Liddy [MacBook9,1] Tested-by: Daniel Roschka [MacBookPro13,2] Tested-by: Ronald Tschalär [MacBookPro13,3] Tested-by: Peter Y. Chuang [MacBookPro14,1] Reviewed-by: Andy Shevchenko Signed-off-by: Ronald Tschalär Signed-off-by: Lukas Wunner Signed-off-by: Marcel Holtmann --- 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 #include #include +#include #include #include #include @@ -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", From 5954cdf1793a0e1296e495884669c79f9ef2b2b3 Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Wed, 10 Jan 2018 16:32:10 +0100 Subject: [PATCH 21/23] Bluetooth: hci_bcm: Silence IRQ printk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Andy Shevchenko Signed-off-by: Lukas Wunner Signed-off-by: Marcel Holtmann --- 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; } From e4b9e5b8618dc59f5de5cecf4e0188a3cdcd5648 Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Wed, 10 Jan 2018 16:32:10 +0100 Subject: [PATCH 22/23] Bluetooth: hci_bcm: Sleep instead of spinning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Suggested-and-reviewed-by: Andy Shevchenko Signed-off-by: Lukas Wunner Signed-off-by: Marcel Holtmann --- 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) { From ff8759609d021c0e85945fcc4a148a0e55ace70f Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Wed, 10 Jan 2018 16:32:10 +0100 Subject: [PATCH 23/23] Bluetooth: btbcm: Fix sleep mode struct ordering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Andy Shevchenko Signed-off-by: Lukas Wunner Signed-off-by: Marcel Holtmann --- 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)