Repository navigation
PCI: apple: train a T6050 root port the boot loader left down (J716s N1) - #1182
RyanTheTide merged 12 commits into
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0e28f1c1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| bus = pcie->cfg->busr.start + 1 + port->idx; | ||
| if (bus > pcie->cfg->busr.end) | ||
| return -EINVAL; |
There was a problem hiding this comment.
When earlier root ports are disabled or rejected, this gives a higher-numbered cold port the wrong secondary bus: port 1 alone gets bus 2. In the normal non-PCI_PROBE_ONLY path, pci_host_common_init() enables PCI_REASSIGN_ALL_BUS, and pci_scan_bridge_extend() assigns the first surviving bridge bus 1 unless it has fixed EA bus numbers. Adoption has already recorded bus 2 in adopted_bus and installed its RID route, so apple_pcie_map_bus() rejects endpoint accesses on the newly assigned bus 1; alternatively, adoption fails earlier if iommu-map already describes bus 1. Choose numbers consistent with the actually admitted ports, or prevent the PCI core from renumbering them.
Useful? React with 👍 / 👎.
| ret = apple_pcie_t6050_cold_train(port); | ||
| if (!ret) | ||
| ret = apple_pcie_adopt_link(port); |
There was a problem hiding this comment.
If cold training succeeds but the second adoption check or subsequent IRQ setup fails, err_teardown calls apple_pcie_port_teardown(), which removes IRQ/RID state but does not assert PERST#, stop the port clocks, or lower the newly acquired pwren. The failed port is then absent from pcie->ports, or the host probe fails altogether, so apple_pcie_shutdown() cannot park that live link either. This can leave precisely the link-alive state that the new shutdown hook is intended to eliminate on restart. Track cold-training ownership and unwind the powered/trained hardware before discarding a failed port.
Useful? React with 👍 / 👎.
| if (!apple_dart_is_available(dev->of_node)) | ||
| return dev_err_probe(dev, -EPROBE_DEFER, "waiting for its client\n"); |
There was a problem hiding this comment.
With fw_devlink=on fw_devlink.strict=1, a manual-availability DART referenced by the host's iommu-map creates a probe deadlock. Strict mode treats inferred dependencies as “mandatory dependencies” (Documentation/admin-guide/kernel-parameters.txt:1831-1833), and drivers/of/property.c parses iommu-map into a host-to-DART supplier link. Consequently, device_links_check_suppliers() prevents apple_pcie_probe() from running until the DART binds, while this new check defers the DART until that same host probe calls apple_pcie_dart_available(). Make the late-provider relationship visible to fw_devlink, for example through a documented post-init-providers requirement, so both devices do not remain deferred indefinitely.
Useful? React with 👍 / 👎.
a873be0 to
2b76075
Compare
a0e28f1 to
089d4cb
Compare
|
The base branch j716c/t6050-pcie was force-pushed (a873be0 to 911da8c), so the head conflicted with it in apple,pcie.yaml, pcie-apple.c and gpio-macsmc.c; rebased onto 911da8c with the conflicts resolved and edimoldovan's commits unchanged in content. The binding also needed the six new reg-names in the second t6050 block that the new base added (maxItems 14 to 20); the nine commits compile clean at W=1 per commit. |
RyanTheTide
left a comment
RyanTheTide
left a comment
There was a problem hiding this comment.
Needed before this can go in:
-
Signed-off-by from the author. These six commits are authored by
ed <edimoldovan@gmail.com>and carry only Rolando Trevino's Signed-off-by: 1fe4cb3, a053aaf, ef54401, 3cd54a5, 9e8ec7e, 5d83e63. Each needs the author's ownSigned-off-by: Eduard Moldovan <edimoldovan@gmail.com>(added by him, e.g.git rebase -x 'git commit --amend --no-edit -s'on his side), with Rolando's kept after it as the person passing them on. -
Base.
j716c/t6050-pcie(911da8c) is already contained in integration/aurora, so this can be retargeted to integration/aurora with the same nine commits. After retargeting it conflicts with integration/aurora inDocumentation/devicetree/bindings/iommu/apple,dart.yamlanddrivers/iommu/apple-dart.c(both sides add next to the same hunks); please rebase onto integration/aurora and resolve those. -
Named fields for the new register writes in
pcie-apple.c:apple_pcie_t6050_cold_train():writel(0x140, pcie->base + CORE_RC_T6050_PRE_RUN)needs the value built from named, commented fields.apple_pcie_t6050_shared_phy_on():T6050_PHY_CFGBIT(0) and BIT(4) andT6050_PHY_STATBIT(0) need names.apple_pcie_shutdown()copies the PME_Turn_Off/L2 part ofapple_pcie_t6050_reset_root_port()(| 0x100on PORT_APPCLK,writel(0x11, 0x88), the poll of 0x88 bit 0, PORT_LINKSTS BIT(6),port->phyBIT(31)). Please move that sequence into one helper used by both, with names for those values (0x100 isPORT_APPCLK_CGDIS).
Lab-board scope checked: the PCIe changes run only with t6050_hw.adopt_link, the DART deferral only with apple,manual-availability, and the SMC GPIO change only on apple,j716s.
|
Signed-off-by: Eduard Moldovan edimoldovan@gmail.com |
…he port On the J714c the one-boot kernel (aurora's adopt path, the N1 in its ROM personality without firmware) froze 20-40 s after boot: user space first, then the whole SoC; a boot with pcie_aspm=off ran clean, a boot with the PCIe node disabled ran clean. The bus notifier that strips ASPM L1 from tunneled root ports now serves adopt_link hosts too and strips L0s and L1 from the adopted root ports, so the link stays in L0. A restart with the adopted link alive never came back to m1n1 (hard reset needed), while the same restart with the PCIe node disabled did. The platform driver gets a shutdown that puts each adopted port back into the boot stage's cold state: bridge decoding off, PME_Turn_Off and L2, PERST# asserted, the port and its lane clocks stopped (the first half of apple_pcie_t6050_reset_root_port, without the restart). Link: edimoldovan/linux-aurora@ab24d156868e Signed-off-by: Eduard Moldovan <edimoldovan@gmail.com> Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com>
A T6050 root port that names its endpoint's power line can be left untrained by the boot loader: the OS powers the endpoint and trains the link itself, then adopts it like a port the boot loader trained. Allow pwren-gpios on such a port, the windows the train uses in reg-names (phy-shared, phy-common, axi2af, pcieclkgen, phy-window, phy-ip) and the six host tunable lists the boot loader copies from the Apple device tree, as (offset, width, mask, value) groups. Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com> Assisted-by: LLM Claude
The adopt path takes a T6050 port only if the boot loader trained its link. iBoot does not on the J714c: its N1 is held off (SMC GPIO gP13 low, PERST# low), the shared PCIe PHY is in reset with its clocks off, and Wi-Fi works only when the port is trained over the m1n1 proxy before the kernel starts. When the adopt checks find the link down and the port names pwren-gpios, train it: the shared PHY clocked and out of reset (phy-shared, phy-common), the RC running, PERST# held and the endpoint powered, then the port enable, configure and train sequences the root-port reset already uses, and the bus numbers a boot loader leaves. The adopt checks then run unchanged. The adopt gate accepts pwren-gpios on such a port. The root-port configure step now reaches the root port through the ECAM window: before enumeration there is no pci_dev. The root-port reset uses the same code. Link: edimoldovan/linux-aurora@0f96cc6c00e1 Signed-off-by: Eduard Moldovan <edimoldovan@gmail.com> Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com>
A port trained without the boot loader's tunables comes up, but the PHY tunables are what clear lane bit 28 (and the shared PHY's bit 26); the adopt checks expect that state, and every proxy-trained boot has had it. Apply the host lists the DT carries (apple,tunable-axi2af, -cio3pllcore, -pcieclkgen, -phy, then -phy-ip-pll and -phy-ip-auspma after the PHY clocks), in m1n1's order. The windows may contain windows mapped elsewhere, so they are mapped without claiming them. Link: edimoldovan/linux-aurora@17ada56a25c9 Signed-off-by: Eduard Moldovan <edimoldovan@gmail.com> Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com>
A DART behind a PCIe root complex answers only once the root complex is up. Mark such a DART so it probes only after its client says so. Link: edimoldovan/linux-aurora@8805d2777c8b Signed-off-by: Eduard Moldovan <edimoldovan@gmail.com> Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com>
A DART marked apple,manual-availability defers its probe until its client calls apple_dart_make_available() for its node. [Rolando: kzalloc_obj(), which this tree prefers] Link: edimoldovan/linux-aurora@dbd43f4ff3da Signed-off-by: Eduard Moldovan <edimoldovan@gmail.com> Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com>
Once a port is adopted, trained by the bootloader or by Linux, the root complex is up: make the port's DART available. Link: edimoldovan/linux-aurora@3291060c462a Signed-off-by: Eduard Moldovan <edimoldovan@gmail.com> Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com>
A restart resets the SoC but not the J716s N1: it keeps running its firmware with its power line (SMC gP13) released. m1n1 found that a 50 ms power cycle does not bring such an N1 back in its ROM personality, and holds the line low for 250 ms when it reads released (the J716s warm-restart runs, three of three). The cold train cannot read the line back, so it always holds it low for 250 ms; a cold boot pays 200 ms. The train takes PERST# and the power line before its first register write: the SMC's GPIO bank probes late, and a probe deferred for it must not leave the shared PHY and the root complex half set up. The J716s N1 got up to 1 s for link up in m1n1 after its power-up; the train now allows the same instead of 500 ms. The root-port reset shares the wait, where only a failure takes longer. A port Linux trained keeps the endpoint's power line, and shutdown drops it after parking the port, so the boot after a restart finds the N1 cold whether its loader trains the port or the kernel does. Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com> Assisted-by: LLM Claude
The J716s N1's power line is /arm-io/apcie0/pci-bridge0 function-reset,
pKW4("gP13", 0x00800000): the descriptor J713's /amfm/function-reg_on
uses. m1n1 trains the N1's port on the J716s by writing that template
with the level in its low bits (0x00800000, then 0x00800001); the
generic CMD_OUTPUT write was never tried on this machine.
Drive gP13 on the J716s as on J713, so the kernel's own train of the
port powers the N1 with the same SMC writes.
Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com>
Assisted-by: LLM Claude
Name the bits the T6050 shared PHY bring-up and the RC run write use, after what a trace of macOS bringing the port up and down shows them doing: PORT_APPCLK_CGDIS (the clock gate disable the driver already defines) for the 0x100 written before PME_Turn_Off, PORT_PME_TURN_OFF for the 0x88 register macOS writes 0x11 to, PORT_LINKSTS_L2 for the link status bit macOS reads as "reached L2", and PHY_LANE_CFG_REFCLKCGEN_B31 for the lane bit it clears after asserting PERST#. The PHY_CFG bits 0 and 4, PHY_STAT bit 0, the PME_Turn_Off bit 4 and the two bits of the 0x140 written to the RC before it runs have no documented meaning, so they get neutral names and a comment with what the traces show. The PME_Turn_Off and L2 wait was written out twice, in the root-port reset and in the shutdown. Move it into apple_pcie_t6050_port_l2(). The register writes, values, order and timeouts are unchanged. Both polls now always run, so a failed first poll in the root-port reset no longer skips the second one (a read-only poll, up to 500 ms on that error path); the shutdown already ran both. Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com> Assisted-by: LLM Claude
089d4cb to
5282665
Compare
|
Updated (head 5282665), retargeted to integration/aurora and rebased onto 0227cec.
All ten commits compile with W=1 and no warnings. Not boot-tested since the rebase. Two of the Codex findings are real and not fixed here, since I cannot test either on the hardware yet: a cold-trained port is not parked if a later setup step fails, and a |
When the driver cold-trains a T6050 port it raises the endpoint's power (pwren), releases PERST# and trains the link. If the second adoption check, the IRQ setup or a later step then fails, err_teardown only releases the IRQ and RID state and drops the port from pcie->ports. The shutdown hook walks that list, so it never parks the port: the link stays up and the endpoint powered across a restart, the state the hook exists to prevent. A failure inside the cold train after the pwren write leaves the same. Remember that the cold train raised pwren (port->powered, set just before the write) and, from err_teardown, put the port back the way the shutdown hook does. The park sequence is now one function, apple_pcie_t6050_port_park(), used by the shutdown hook and by the failure path: PME_Turn_Off to L2, PERST# asserted, the lane clock request off, the port disabled and its clocks stopped, the endpoint's power off. The unwind runs after apple_pcie_port_teardown(), which still writes the port's registers and so needs the port clocked. What runs depends on how far the cold train got: - Failure before the pwren write: nothing. pwren was requested low and no port register was written, so there is nothing to undo. - Link not up (the enable, configure or train step failed): no PME_Turn_Off, which has no one to answer and would only wait out two 500 ms timeouts; then the rest of the sequence. The disable is best effort on a port that was never fully enabled: its first wait may time out, in which case the endpoint is still powered off. - Link up (the second adoption check, IRQ setup or a later step failed): the full sequence as at shutdown, after clearing the bus numbers the cold train wrote, as the hook does. The shutdown hook runs the same steps in the same order as before. The disable no longer takes a pci_dev: it read the root port's LTSSM state through config space, which does not exist during probe, and now reads the same register through the ECAM window as the rest of the cold train does. The success path and the adopt-only (loader-trained) path are not changed. Compile-tested only. Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com> Assisted-by: LLM Claude
…availability DART A DART marked apple,manual-availability defers its probe until the PCIe host calls apple_dart_make_available() from its own probe. The host's iommu-map makes fw_devlink create a host to DART supplier link; iommu-map is an optional supplier, enforced only under fw_devlink.strict=1, where the host then waits for the DART while the DART waits for the host and neither probes. post-init-providers is the property for this: fw_devlink ignores the link to a supplier listed in it (the flag is merged onto the link that iommu-map created). Allow it on the host node and say in the apple,manual-availability description that the host must list the DART there. No driver change is needed. Compile-tested only. Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com> Assisted-by: LLM Claude
|
Two more commits on top (head 22dcb0f) for the two Codex findings I had left open. Both are compile-tested only; the J716s is busy, and I will boot them and report here.
All twelve commits compile with W=1 and no warnings; |
febb624
into
aurora-silicon:integration/aurora
On the M5 Pro/Max laptops iBoot leaves apcie0 port 0, the N1 Wi-Fi/Bluetooth, untrained: the N1 is held off (SMC GPIO gP13 low, PERST# low), the shared PCIe PHY is in reset with its clocks off, and the port's power states are gated. #937 adopts a port only if the boot loader trained it, so until now Wi-Fi needed a loader that trains the port (on the J716s our m1n1 does, with ANS started for its storage checks and an SMC reset pulse). edimoldovan's J714c tree lets the kernel train the port itself when the adoption checks find the link down and the port names its endpoint's power line; the adoption checks then run unchanged. This brings that work here, with what the J716s (MacBook Pro 16-inch, M5 Pro) needs on top.
Commits:
PCI: apple: T6050 adopted ports: no ASPM, and a shutdown that holds the port(ed). ASPM stripped from adopted root ports, and a shutdown that parks each adopted port as the boot stage leaves it. Without it the J716s freezes about 30 s after the N1 firmware starts unlesspcie_aspm=offis set, and a restart with the link alive does not come back. This supersedes the ASPM-only change I had held for PCI: apple: adopt the T6050 root ports the boot loader trained #937.dt-bindings: pci: apple,pcie: Let the OS train a T6050 root port.pwren-gpioson a T6050 root port, the new reg-names and the six host tunable lists.PCI: apple: T6050 adopt: train a port the boot loader left down(ed). The shared PHY is brought up (clocked and out of reset), then the RC run bit, PERST# held, the endpoint powered, then PCI: apple: adopt the T6050 root ports the boot loader trained #937's port enable, configure and train sequences, then the bus numbers. Root-port configure now reaches the root port through ECAM, because there is no pci_dev before enumeration.PCI: apple: T6050 cold train: apply the host tunables(ed).apple,tunable-{axi2af,cio3pllcore,pcieclkgen,phy,phy-ip-pll,phy-ip-auspma}in m1n1's order. Thephylist is what clears lane bit 28, which the adoption checks require.dt-bindings: iommu: apple,dart: add apple,manual-availability(ed).iommu: apple-dart: probe a manual-availability DART after its client(ed). Merged next to the J713 display-DART checks inapple_dart_probe().PCI: apple: make an adopted port's DART available(ed). The DART of a port that is not up takes an SError when probed. With this it can be enabled in the DT and still probe only after the host has the port up.PCI: apple: T6050 cold train: reset a warm N1, power it off at shutdown. A restart resets the SoC but not the N1, which keeps running its firmware. The train now holds the power line low for 250 ms (the 50 ms of commit 3 does not reset a warm N1; m1n1 measured 250 ms on the J716s). It takes PERST# and the power line before its first register write, so that a probe deferred for the late SMC GPIO bank leaves the hardware untouched. The link-up wait is 1 s, as m1n1 allows. Shutdown drops the power line after the park when Linux trained the port.gpio: macsmc: drive the J716s N1 power line with its ADT template. The J716s ADT gives the N1's power line as pKW4("gP13", 0x00800000), the same descriptor as J713's radio. It is written as0x00800000 | level, the write m1n1 uses on this machine, instead of the genericCMD_OUTPUT(which the J714c uses).The J716s DT change goes on top of #1167, because the J716s DT is not in this tree: branch
n1-kernel-train-dt= #1167 + commit 5 +arm64: dts: apple: t6050-j716s: let the kernel train the N1's port. That commit adds the shared PHY, PHY common block and the four tunable windows to pcie0,pwren-gpios = <&smc_gpio 0x13>on port00, the SMC's first GPIO bank (gP keys),apple,manual-availabilityon dart_apcie0, and the port's power chain in pmgr1. iBoot leaves APCIE_SYS_GP, APCIE_PHY_SW and APHY_GP_AUS5 in auto-PM with the block gated, the same on the J714c. In that commit they are always on with auto-PM off, rather than genpd-managed as in the J714c tree: the adopted link is kept up through suspend-to-idle, and genpd would power a consumer's domain off in system sleep. I can add it to #1167 instead if you prefer.Loader contract. port00 and dart_apcie0 stay disabled in the DT. The loader enables them when the port is up, or when the port is down but names
pwren-gpiosand its DART hasapple,manual-availability. The loader copies the six host lists from the ADT (apcie-<name>-tunables, cio3pllcore moved into the pcieclkgen window at +0x2200 as in the J714c tree) next to the port lists it already copies. These values differ between the J716s and the J714c (the PHY PLL fields), so they cannot be static. That is the m1n1 side (follow-up to aurora-silicon/m1n1#65): with it, the m1n1 N1 training (ANS start, SMC pulse, warm-entry handling) is no longer needed.Tested on: J716s, kernel
7.3.0-g50d99dcea7b4= our J716s series (#937, #938, #1167 and the T6050 PRs) + these commits. The patches here are the same, rebased, with two cosmetic changes: kzalloc_obj() in the DART change and a shorter field comment. Boots self-testing from the primary slot, battery on, nopcie_aspm=off:n1: port 0: rc ctl 0x0 stat 0x1, port status 0x6 linksts 0xc300220c ltssm 0x0,n1: port 0 not up: root port and DART okay. Kernel:systemctl rebootwith wlan0 in use): the next boot trains the port again (same lines, 1.036 s), Wi-Fi and Bluetooth back.Checks (omacom's
tools/aurora-ci, run locally against this base):include/linux/soc/apple/dart.h.n1-kernel-train-dton arm64: dts: apple: add the J716s (MacBook Pro 16-inch, M5 Pro) #1167): t6050-j716s.dtb builds and its dtbs_check has no new warnings. dt-check reports no new blocking problems, but its dt-validate step fails at base and head alike (KeyError 'uint8-array', from conflictingapple,tunabletypes in three bindings). Advisory only:gpio@67500000has a unit name without reg (the same pattern as the existing gp bank atgpio@67700000).Dependencies: #937: branch
n1-kernel-trainis stacked on its head a873be0 (j716c/t6050-pcie), so this PR targetsj716c/t6050-pcieand can be retargeted to integration/aurora once #937 lands. Nothing in integration/aurora adopts a T6050 link without it. The DT half depends on #1167 and on commit 2 here.Authorship: commits 1, 3-7 are edimoldovan's (from
t6050-j714con his fork, where the cold train has run on the J714c). They carry his authorship but no Signed-off-by of his own, only mine. @edimoldovan, could you add your Signed-off-by, or say if you would rather send them yourself?Open questions:
CMD_OUTPUTon the J714c. Is the template right for both machines? Then the J714c could take the same path.