spi: spi-apple: Implement Runtime PM support#530
Conversation
|
This makes a kernel panic...patching soon. |
|
Tested on Apple MacBook Pro (13-inch, M1, 2020). done |
|
Can you tell me how you tested it? |
|
good question. first, the test result after pressing F2 at a touchbar: LogsScript#!/bin/bash
# Check if running as root
if [ "$EUID" -ne 0 ]; then
echo "Error: Please run as root (sudo ./check.sh)"
exit 1
fi
# Find the spi-apple device directory
SPI_DIR=$(find /sys/bus/platform/devices/ -name "*.spi" | head -n 1)
if [ -z "$SPI_DIR" ]; then
echo "Error: apple-spi device not found in sysfs."
exit 1
fi
echo "=================================================="
echo "Target SPI Device: $(basename "$SPI_DIR")"
echo "Path: $SPI_DIR"
echo "=================================================="
# 1. Enable Dynamic Debug for Runtime PM and spi-apple
echo "Enabling dynamic debug logs..."
if [ -f /sys/kernel/debug/dynamic_debug/control ]; then
echo 'file drivers/base/power/runtime.c +p' > /sys/kernel/debug/dynamic_debug/control
echo 'file drivers/spi/spi-apple.c +p' > /sys/kernel/debug/dynamic_debug/control
echo "-> Dynamic debug enabled."
else
echo "-> Warning: /sys/kernel/debug/dynamic_debug/control not found."
echo " Make sure CONFIG_DYNAMIC_DEBUG is enabled in your kernel."
fi
# 2. Check current configuration
echo "--------------------------------------------------"
echo "Initial Runtime PM Configuration:"
echo "--------------------------------------------------"
if [ -f "$SPI_DIR/power/control" ]; then
echo "Power Control Mode: $(cat "$SPI_DIR/power/control")"
else
echo "Power Control Mode: N/A"
fi
if [ -f "$SPI_DIR/power/autosuspend_delay_ms" ]; then
echo "Autosuspend Delay : $(cat "$SPI_DIR/power/autosuspend_delay_ms") ms"
else
echo "Autosuspend Delay : N/A"
fi
echo "--------------------------------------------------"
# 3. Monitor state changes
echo "Starting real-time Runtime PM status monitoring..."
echo "Press [CTRL+C] to stop."
echo ""
echo "Timestamp | Status"
echo "--------------------------------------------------"
while true; do
STATUS=$(cat "$SPI_DIR/power/runtime_status" 2>/dev/null)
TIMESTAMP=$(date +"%Y-%m-%d %H:%M:%S.%3N")
if [ "$STATUS" != "$LAST_STATUS" ]; then
echo "$TIMESTAMP | $STATUS"
LAST_STATUS=$STATUS
fi
sleep 0.05
done |
|
A feature is okay after resume, no long delay when using touch bars...I will submit a full dmesg after fixing another PR's bluetooth power manager |
Signed-off-by: Lee Yunjin <gzblues61@gmail.com>
underflow detected. Currently fixing |
|
root@fedora:/home/yjlee# dmesg | grep apple-spi now clean...yes |
|
@WhatAmISupposedToPutHere It seems okay, please kindly review this when you have enough time |
|
Does it actually do anything? I don't think that clock can be turned off. |
|
Even if the PMGR power domain for this specific SPI block cannot be completely powered down to a 0V state, implementing Runtime PM here provides clear and definite benefits. I'll provide a test code. |
Which does nothing
No, it does not, the clock is not actually disabled.
No, it is not, the clock may have a zero refcount, but it is still active, as it can't be switched off. |
|
Let me recheck the hardware. |
|
Here is the raw experimental output and ftrace data showing how the hardware clock state changes during a touch bar interaction under this patch, Raw Experiment OutputAnalysis of the Trace & Clock ArchitectureThe Shared Parent Clock (
|
|
I agree with your analysis on the hardware clock limitation, so let's focus on the architectural benefits instead. This PR aligns the driver with upstream SPI subsystem standards and ensures the device dependency tree is correct during system-wide suspend/resume transitions. However, if maintaining this PM infrastructure isn't considered a priority for this driver, I am completely fine with closing this PR. |
|
@WhatAmISupposedToPutHere I am not good at writing or reading English, so I sometimes uses GenAI comments to reply. I understand your opinion for this pull request, but I am pretty sure that my bcm4377 patch will actively solve a problem |
Implementing Runtime PM support...
Just a small change