Repository navigation
Conversation
DjDeveloperr
left a comment
There was a problem hiding this comment.
[1/7] pmdomain: review at 8f762ae
Gating: fine. The new behaviour is DT opt-in. apple,bridge-link is set only on the two J713 dispext sys states, and consumer-switched/retention only in t8132-j713-pmgr.dtsi. The T8132 platform handoff copies only status and the two ownership flags from that file, so none of these reach other T8132 boards. Nothing changes for T6050/T8140/T60xx. apple_pmgr_ps_verify_state() is a new export and leaves existing callers alone. This file has no changes on the M5 branch, so no conflict.
Findings
- Abort path (pre-existing, now load-bearing):
apple_pmgr_ps_consumer_on()returns at the first state that is not off. After aconsumer_off()that failed partway, for example fe reading-EBUSYbecause it isn't fully on, the parent-first on-walk gets-EALREADYfrom sys and stops. cpu stays off. #1420 needs exactly this rollback (see there). apple,bridge-linkis a raw physical address of another block's registers inside a PMGR pwrstate node. Upstream binding review will push back. A phandle to the dcpext node plus an offset carries the same information.apple_pmgr_ps_verify_state()appliespd_to_genpd()to anydev->pm_domainand dereferences the result before it knows it is a genpd. That's fine for today's ACIO callers, but cheap to guard on an export.- Nit: the bridge
dev_infofires twice per DCP per sleep, while the neighbouring power-off/on messages aredev_dbg.
Verdict: OK with minor changes. Fix (1) before #1420 relies on a rollback.
Review only: read at the head commit, not built or run (no J713 here). Written with Claude Code.
| - description: value while the domain is off | ||
| maxItems: 4 | ||
|
|
||
| apple,bridge-link: |
There was a problem hiding this comment.
A physical address of another block's registers in a PMGR node is the part an upstream binding review will object to. Something like apple,bridge-link = <&dcpext0 0x104> (phandle plus offset), resolved through the dcpext's reg, keeps it per-board in DT and doesn't hard-code the SoC map in a property.
|
|
||
| if (!domain_dev->pm_domain) | ||
| return -EINVAL; | ||
| genpd = pd_to_genpd(domain_dev->pm_domain); |
There was a problem hiding this comment.
pd_to_genpd() is a bare container_of(), and genpd->power_on is read before we know pm_domain belongs to a genpd. All current callers pass ACIO genpd virtual devices, so this is safe today. Since it's exported, a cheap guard first (for example comparing domain_dev->pm_domain->ops.runtime_suspend with what genpd installs, or keeping a list of this driver's genpds) would keep a wrong caller from reading arbitrary memory.
| /* Complete the stores before the state word changes or its block is used. */ | ||
| mb(); | ||
| readl(ps->bridge); | ||
| dev_info(ps->dev, "PS %s: bridge link %s\n", ps->genpd.name, up ? "up" : "down"); |
There was a problem hiding this comment.
Nit: this logs twice per external DCP on every sleep. The consumer power-off/on messages next to it are dev_dbg.
| ret = apple_pmgr_ps_wait(ps, APPLE_PMGR_PS_LEVELS, APPLE_PMGR_PS_ALL_ON, &cur); | ||
| if (ret) | ||
| goto fail; | ||
| apple_pmgr_ps_bridge(ps, true); |
There was a problem hiding this comment.
Related, though not in this diff: apple_pmgr_ps_consumer_on() walks parent-first and returns on the first error, and consumer_state_on() gives -EALREADY for a state that isn't off. If consumer_off() failed partway (cpu off, then fe -EBUSY because it wasn't fully on), the rollback consumer_on() stops at sys and never powers cpu back on. #1420 needs this rollback on its failed-stop path. Suggest treating -EALREADY as done in the on-walk, or recording which states this call switched.
A consumer-switched power state can have a bus bridge in front of the block it powers. The bridge's two link words must have bit 0 cleared before the state powers off and set again once it is on, and they are only accessible while the state is on. Describe where they are: a phandle to the block's node, the index of its register range and the offset of the words in that range. The PMGR driver can then do that as part of the consumer's power-off and power-on. The T8132 external display system domains need it. Signed-off-by: Erik Bourget <erik@ebourget.net>
A T8132 external display system domain must not be powered off with the bus bridge of its block still linked. When a consumer-switched state has apple,bridge-link, clear bit 0 of the two link words just before the state powers off, and set it again once the state reads fully on, before the retention words are restored. The words are touched only while the state is on. Their address comes from the named block's register range and the offset in the property. This extends the consumer-switched power states. Based-on-work-by: Bartosz Nitka <niteria@gmail.com> Signed-off-by: Erik Bourget <erik@ebourget.net>
A driver that takes its power domains down and up itself around a system sleep, through the genpd suspend and resume helpers, has to know that each one really reached the state it asked for before it goes on: the next step either removes power from, or touches, the block behind it. Add a helper that polls the state word of a virtual power-domain device for fully on or fully off. It refuses a device whose power domain is not a genpd, and firmware-owned and inherited-on states, which the driver does not control. Signed-off-by: Erik Bourget <erik@ebourget.net>
…er power-off apple_pmgr_ps_consumer_on() walks the consumer's chain parent first and stops at the first state it cannot switch. A state that is not off gives -EALREADY. After a consumer power-off that stopped partway, for example with the CPU state off and the frontend refusing because it was not fully on, a rollback power-on stops at the system state, which never went off, and leaves the CPU state off. Treat -EALREADY as done in the power-on walk, so that a rollback after a partial power-off switches back on every state that went off. A consumer rolling back also has to know whether its own block went through reset, which decides how its coprocessor is started again. Add apple_pmgr_ps_consumer_is_off() for that: whether the first state of its chain, its own domain, was powered off. Signed-off-by: Erik Bourget <erik@ebourget.net>
…ains A deep sleep powers the external display coprocessors off, and they have to be stopped first and started again afterwards. Mark the CPU, front-end and system domains of both as consumer-switched, with the CPU domains' power-gate retention words and the system domains' bridge links, so the coprocessor's driver can power the chain off after it has stopped the firmware and on again before it restarts it. The bridge links name the external DCP whose block they are in, so they live in the board file; the PMGR file is also built into the platform data, which has no DCP nodes. The domains stay firmware-owned otherwise: nothing changes at runtime. Based-on-work-by: Bartosz Nitka <niteria@gmail.com> Signed-off-by: Erik Bourget <erik@ebourget.net>
RyanTheTide
left a comment
There was a problem hiding this comment.
This 7-PR stack (#1414-#1420) sits on the J713 radio stack (#1378-#1385), which sits on #1314. #1314 has commits without Signed-off-by and conflicts with integration/aurora, so none of these can merge yet. Either #1314 gets Signed-off-by and a rebase, or rebase this stack and the radio stack onto integration/aurora without #1314's WIP commits. #1415 and #1419 also have failing/cancelled CI runs to re-run after that.
8f762ae to
fe1184f
Compare
|
@erikwb This series is now carried in #1314, rebased onto integration/aurora with your sign-off kept: 10 of the 18 commits are identical and the rest differ only in context and host-test fixtures. One code difference: in tb_switch_suspend_cio() the copy in #1314 checks linux#1409 techmore ci: sep-source: cancel superseded runs and use a read-only token linux#1370 boredcoder411 tools/asahi: Reject missing DART functions with selected cases linux#1369 AxxzyWasTaken soc: apple: clpc, drm/asahi: g17: busy-share memory floor and light effort for T6041
linux#1242 rolotrealanis98 drm/asahi: g17: Name the cause and the clients of every M5 GPU recovery
linux#1477 fbalazsy J616s (M4 Pro) at EL2: hibernation and suspend-to-RAM (status only)
linux#1314 niteria Deep sleep (suspend-to-RAM) for the M4 MacBook Air (T8132, J713) at EL2 (status only) |
The J713's external display coprocessors are powered off by a deep sleep, so their driver has to power their CPU, front-end and system domains off after stopping the firmware and on again before restarting it. The system domain has a bus bridge in front of its block whose link must go down before the domain powers off and come back up once it is on.
apple,bridge-linkto the PMGR power-state binding (a phandle to the block, the index of its register range and the offset of the link words) and handle it in the consumer-switched power-off and power-on.apple_pmgr_ps_verify_state(), used later in the series by the ACIO host to confirm each of its domains reached the state it asked for. It refuses a device whose power domain is not a genpd.apple_pmgr_ps_consumer_is_off()says whether the consumer's own domain went off, i.e. whether its block went through reset.Part 1/7 of J713 docked S2R, stacked on the native radio series (#1384), which is stacked on #1314.
Series order: #1414 → #1415 → #1416 → #1417 → #1418 → #1419 → #1420.
Depends on #1384.
Validation of the series on this base:
Based-on-work-by:trailers and style in thetools/asahifixtures.The changes since that version (the DT-selected DART owners and memory-only updates while closed, the unplug handling, the failure unwinds, the stock tb.c resume restored, the PMGR and RTKit hardening) are built and covered by the host suites, but have not run on the J713 yet.
Not covered: repeated sleeps on one boot, wake sources other than the RTC, the desktop's own sleep path (lid, logind), a dock unplugged during the sleep on hardware, and docks other than the TS5. Both native parameters (
pcie_apple.cio_s2r,thunderbolt_apple.cio_s2r) default to off.This series was developed and tested by Claude (Anthropic), working with me on the J713.