Return-Path: Subject: Re: [PATCH v2 03/10] Bluetooth: hci_bcm: Enable runtime PM despite absence of IRQ To: Lukas Wunner Cc: Marcel Holtmann , Johan Hedberg , Mika Westerberg , Andy Shevchenko , Frederic Danis , Loic Poulain , Max Shavrick , Leif Liddy , Daniel Roschka , Ronald Tschalaer , "Peter Y. Chuang" , linux-bluetooth@vger.kernel.org References: <36e7c22493235863c2ab9dd9d7aa382a3fc15675.1514916630.git.lukas@wunner.de> <20180102233648.GA15299@wunner.de> From: Hans de Goede Message-ID: <89642ecf-63b0-1aa7-8df0-ff4ecb5efe2d@redhat.com> Date: Wed, 3 Jan 2018 09:07:20 +0100 MIME-Version: 1.0 In-Reply-To: <20180102233648.GA15299@wunner.de> Content-Type: text/plain; charset=utf-8; format=flowed List-ID: Hi, On 03-01-18 00:36, Lukas Wunner wrote: > On Tue, Jan 02, 2018 at 08:17:24PM +0100, Hans de Goede wrote: >> On 02-01-18 20:08, Lukas Wunner wrote: >>> Currently runtime PM is only enabled if a valid host wake IRQ was found. >>> >>> However it is used in ways which seem unrelated to host wake: >>> E.g., runtime PM is used to force the Bluetooth device on for 5 sec >>> after a complete packet has been received. >>> >>> Afford this functionality to devices which lack an IRQ by moving the >>> code to enable runtime PM from bcm_request_irq() to bcm_setup(). >> >> Hmm, this seems to not make much sense. If runtime_pm is not enabled >> (no host wake irq) then the device never runtime suspends, so there >> is no need to force the device on when we're sending / after a >> complete packet has been received. >> >> That you give "after a complete packet has been received" as condition >> example illustrates nicely why I believe this commit is a bad idea, >> if say a bluetooth keyboard wants to send a keypress HID report after >> the runtime pm timeout has elapsed, how are we going to receive that >> report since we're runtime suspended and there is no host wake irq >> to tell us to wakeup the UART and enable receiving on it again? > > The host UART on MacBooks apparently never runtime suspends, so this > couldn't have been observed during testing: > > They use either 8250_lpss.c (which has no runtime PM callbacks) > or 8250_dw.c (which disables the clock during runtime suspend, > but the MacBooks use a fixed rate clock which can't be disabled). > Moreover intel_lpss_suspend() excludes the UART from being put > into reset. > > But the objection is valid so I withdraw this patch. Ok, thanks. > On MacBooks the host wake pin is ORed with the wake pin of the trackpad > and keyboard and goes into the SMC. I think it is only meant to be > used to wake the system from sleep and not to provide an interrupt > at runtime. In theory we could request the SMC's GPE but we couldn't > tell if the interrupt was sent by the trackpad/keyboard or by the > Bluetooth controller, or was caused by something else entirely. > > So it's probably better not to use runtime PM at all. Let me rework > patch [10/10] and remove this hunk: > > + /* Macs wire the host wake pin to the System Management Controller, > + * which handles wakeup independently of the operating system. > + */ > + if (x86_apple_machine) { > + err = 0; > + goto unlock; > + } > + > > I notice that command 0xfc27 is used to write the sleep_params. > Does anyone know the command to read them so that we can check the > defaults Apple is using and find out if they can be optimized? > > >> I don't see how runtime pm can work without a host-wake IRQ, >> so IMHO this commit is wrong. Tip when testing this, make sure >> that you don't have the bluetooth config panel of e.g. gnome3 >> open as that continuously scans for new devices, so runtime pm >> never kicks in. I've had a bad configured host IRQ pin on some >> ASUS laptops using serdev bcm bt and the only way a bt keyboard >> would work reliable was to keep the config panel open... > > I don't have one of these MacBooks, I'm submitting patches on > behalf of others, I didn't realize that, great work on this! It must not be easy to develop a comprehensive patch set like this without direct hw access. > but I'm told that having the gnome panel open > leads to choppy audio on these machines. Is that normal? I've never really tested this. Regards, Hans