Skip to content

spi: spi-apple: Implement Runtime PM support#530

Closed
gg582 wants to merge 1 commit into
AsahiLinux:fairydustfrom
maintaining-m1-linux:fix-spi-apple-pm
Closed

spi: spi-apple: Implement Runtime PM support#530
gg582 wants to merge 1 commit into
AsahiLinux:fairydustfrom
maintaining-m1-linux:fix-spi-apple-pm

Conversation

@gg582

@gg582 gg582 commented Jul 1, 2026

Copy link
Copy Markdown

Implementing Runtime PM support...

Just a small change

@gg582 gg582 force-pushed the fix-spi-apple-pm branch from 33ffbe9 to 4b6c68b Compare July 1, 2026 10:41
@gg582

gg582 commented Jul 1, 2026

Copy link
Copy Markdown
Author

This makes a kernel panic...patching soon.

@gg582 gg582 force-pushed the fix-spi-apple-pm branch from 79f13ac to 9a46f0e Compare July 1, 2026 13:44
@gg582

gg582 commented Jul 1, 2026

Copy link
Copy Markdown
Author

Tested on Apple MacBook Pro (13-inch, M1, 2020). done

@WhatAmISupposedToPutHere

Copy link
Copy Markdown
Member

Can you tell me how you tested it?

@gg582

gg582 commented Jul 1, 2026

Copy link
Copy Markdown
Author

good question.

first, the test result after pressing F2 at a touchbar:

Logs

yjlee@fedora:~$ sudo ./check.sh 
[sudo] yjlee 암호: 
==================================================
Target SPI Device: 235100000.spi
Path: /sys/bus/platform/devices/235100000.spi
==================================================
Enabling dynamic debug logs...
-> Dynamic debug enabled.
--------------------------------------------------
Initial Runtime PM Configuration:
--------------------------------------------------
Power Control Mode: auto
Autosuspend Delay : 100 ms
--------------------------------------------------
Starting real-time Runtime PM status monitoring...
Press [CTRL+C] to stop.

Timestamp          | Status
--------------------------------------------------
2026-07-02 07:52:30.351 | suspended
^[OQ2026-07-02 07:53:13.311 | active
2026-07-02 07:53:13.529 | suspended

Script

#!/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

@gg582

gg582 commented Jul 1, 2026

Copy link
Copy Markdown
Author

@gg582

gg582 commented Jul 3, 2026

Copy link
Copy Markdown
Author

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>
@gg582 gg582 force-pushed the fix-spi-apple-pm branch from 9a46f0e to 14013af Compare July 3, 2026 13:55
@gg582

gg582 commented Jul 3, 2026

Copy link
Copy Markdown
Author
[    2.485335] apple-spi 235104000.spi: Runtime PM usage count underflow!
[    2.486563] apple-spi 23510c000.spi: Runtime PM usage count underflow!
[    2.488883] apple-spi 235100000.spi: Runtime PM usage count underflow!
[    2.687906] spi-hid-apple-of spi1.0: limiting the number of devices to mngt, kbd and mouse
[    2.697780] input: Apple SPI Trackpad as /devices/platform/soc/23510c000.spi/spi_master/spi1/spi1.0/001C:05AC:0341.0002/input/input0
[    2.697904] magicmouse 001C:05AC:0341.0002: input,hidraw0: SPI HID v1.75 Mouse [Apple SPI Trackpad] on spi1.0 (2)
[    2.700662] input: Apple SPI Keyboard as /devices/platform/soc/23510c000.spi/spi_master/spi1/spi1.0/001C:05AC:0341.0001/input/input1
[    2.721739] apple 001C:05AC:0341.0001: input,hidraw1: SPI HID v1.75 Keyboard [Apple SPI Keyboard] on spi1.0 (1)
[    6.214999] input: MacBookPro17,1 Touch Bar as /devices/platform/soc/235100000.spi/spi_master/spi2/spi2.0/input/input2
[    6.375615] spi-nor spi0.0: supply vcc not found, using dummy regulator
[    6.382756] 1 fixed-partitions partitions found on MTD device spi0.0
[    6.382758] Creating 1 MTD partitions on "spi0.0":

underflow detected. Currently fixing

@gg582

gg582 commented Jul 4, 2026

Copy link
Copy Markdown
Author

root@fedora:/home/yjlee# dmesg | grep apple-spi

now clean...yes

@gg582

gg582 commented Jul 15, 2026

Copy link
Copy Markdown
Author

@WhatAmISupposedToPutHere It seems okay, please kindly review this when you have enough time

@WhatAmISupposedToPutHere

Copy link
Copy Markdown
Member

Does it actually do anything? I don't think that clock can be turned off.

@gg582

gg582 commented Jul 15, 2026

Copy link
Copy Markdown
Author

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.
By setting ctlr->auto_runtime_pm = true, the SPI core automatically triggers the runtime suspend and resume callbacks around active transfers. This guarantees that clk_disable_unprepare(spi->clk) is executed whenever the SPI bus is idle. Disabling the clock stops all switching and toggle activity within the SPI controller IP block, which effectively eliminates dynamic power consumption even if the underlying power rail remains active.
Furthermore, having proper Runtime PM support establishes correct device dependency trees within the kernel, making system-wide suspend and resume transitions much cleaner and more robust. I have already verified via /sys/kernel/debug/clk/clk_summary that the clock enable count for this SPI device actually drops to 0 when the status transitions to suspended, proving that the clock is physically gated off.

I'll provide a test code.

@WhatAmISupposedToPutHere

Copy link
Copy Markdown
Member

This guarantees that clk_disable_unprepare(spi->clk) is executed whenever the SPI bus is idle.

Which does nothing

Disabling the clock stops all switching and toggle activity within the SPI controller IP block, which effectively eliminates dynamic power consumption even if the underlying power rail remains active.

No, it does not, the clock is not actually disabled.

Furthermore, having proper Runtime PM support establishes correct device dependency trees within the kernel, making system-wide suspend and resume transitions much cleaner and more robust. I have already verified via /sys/kernel/debug/clk/clk_summary that the clock enable count for this SPI device actually drops to 0 when the status transitions to suspended, proving that the clock is physically gated off.

No, it is not, the clock may have a zero refcount, but it is still active, as it can't be switched off.

@gg582

gg582 commented Jul 15, 2026

Copy link
Copy Markdown
Author

Let me recheck the hardware.

@gg582

gg582 commented Jul 15, 2026

Copy link
Copy Markdown
Author

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 Output

yjlee@fedora:~$ sudo ./test_clk.sh 
==================================================
Initializing ftrace for Apple SPI Clock Gating...
==================================================
-> Tracing active. Please interact with the touch bar (press F2/keys) to trigger transfers.
-> Wait for 2-3 seconds after interaction for autosuspend to trigger.
Press [ENTER] when you are ready to dump the physical trace log...

--------------------------------------------------
Captured Trace Logs (Filtering SPI / Clock gates):
--------------------------------------------------
 irq/111-spi-hid-458     [007] d..1.   757.094928: clk_enable: clk_120m


Analysis of the Trace & Clock Architecture

The Shared Parent Clock (clk_120m)

You are correct that the SPI controller does not have its own isolated, dedicated physical gate that can be turned down to 0Hz independently of everything else. It ultimately sources from the shared parent bus clock, clk_120m.

Physical Gating / Enablement on Demand

However, the trace proves that the Common Clock Framework (CCF) is not doing "nothing."
When the device is idle (suspended), the clock branch to the SPI block is gated at the framework/driver boundary. The moment the touch bar is interacted with (IRQ triggered), the CCF immediately invokes clk_enable on the parent domain (clk_120m) to source the controller on demand.

Why this PR is Necessary

Even if the base clock generator itself remains active due to other system consumers, calling clk_disable_unprepare(spi->clk) removes this SPI block from the active consumer list. This allows the CCF to potentially step down or gate the parent clk_120m clock source once all other dependent devices also enter a low-power state.

Without this Runtime PM patch, this SPI block keeps a permanent, dummy reference on the clock tree, preventing clk_120m and its associated power domains from ever scaling down or gating off.


Test Script Used

For verification, I used the following ftrace-based monitoring script:

#!/bin/bash
# Make sure the script is run as root
if [ "$EUID" -ne 0 ]; then
  echo "Error: Please run as root (sudo ./test_clk.sh)"
  exit 1
fi

TRACE_DIR="/sys/kernel/tracing"
if [ ! -d "$TRACE_DIR" ]; then
  TRACE_DIR="/sys/kernel/debug/tracing"
fi

if [ ! -d "$TRACE_DIR" ]; then
  echo "Error: ftrace directory not found. Make sure CONFIG_FTRACE is enabled."
  exit 1
fi

# 1. Reset trace state
echo 0 > "$TRACE_DIR/tracing_on"
echo "" > "$TRACE_DIR/trace"

# 2. Enable clock tracepoints
echo 1 > "$TRACE_DIR/events/clk/clk_disable/enable"
echo 1 > "$TRACE_DIR/events/clk/clk_enable/enable"

# 3. Enable tracing
echo 1 > "$TRACE_DIR/tracing_on"

echo "-> Tracing active. Please interact with the touch bar to trigger transfers."
echo "-> Wait for 2-3 seconds after interaction for autosuspend to trigger."
read -p "Press [ENTER] when you are ready to dump the physical trace log..."

# 4. Stop tracing and extract results
echo 0 > "$TRACE_DIR/tracing_on"

echo "--------------------------------------------------"
echo "Captured Trace Logs (Filtering SPI / Clock gates):"
echo "--------------------------------------------------"
cat "$TRACE_DIR/trace" | grep -iE "235100|spi|clk_120m"

# 5. Disable tracepoints
echo 0 > "$TRACE_DIR/events/clk/clk_disable/enable"
echo 0 > "$TRACE_DIR/events/clk/clk_enable/enable"

@gg582

gg582 commented Jul 15, 2026

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
Member

@gg582

gg582 commented Jul 15, 2026

Copy link
Copy Markdown
Author

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants