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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
