Repository navigation
arm64: dts: apple: t6050-j716s: let the kernel train the N1 root port (on #1167) - #1183
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: 2fdad550ce
ℹ️ 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".
| smc_gpio: gpio@67500000 { | ||
| compatible = "apple,smc-gpio"; | ||
| #gpio-cells = <2>; | ||
| gpio-controller; |
There was a problem hiding this comment.
When the loader leaves port 0 down for kernel training, pwren-gpios uses this bank, but the required native command template is absent. In drivers/gpio/gpio-macsmc.c, the hard-coded gP13 handling only applies to T8132 boards; J716s instead reaches the generic macsmc_gpio_set() branch and writes 0x01000000 | level, not the documented 0x00800000 | level. Thus the new path does not issue the required N1 power-cycle command before link training, potentially leaving Wi-Fi and Bluetooth unavailable. Add apple,output-templates = <0x13 0x00800000> to this provider so the existing driver selects the native command.
Useful? React with 👍 / 👎.
| apple,discover-temperatures; | ||
| /* macsmc_soc_die: the SoC hotspot the SMC computes (TCMb, 47.8 C at idle) */ | ||
| apple,soc-die-keys = "TCMb"; |
There was a problem hiding this comment.
On J775d, this hwmon node remains enabled and retains both properties. When the documented readable float TCMb key is present, macsmc_hwmon_populate_sensors() registers it and increments hwmon->temp.count; macsmc_hwmon_discover_temperatures() then rejects that nonzero count with -EINVAL in drivers/hwmon/macsmc-hwmon.c. This fails the entire hwmon probe, removing fan and DC-power monitoring as well as temperatures. J716s explicitly removes apple,soc-die-keys to avoid this conflict, but J775d needs an equivalent resolution or driver support for combining the two modes.
Useful? React with 👍 / 👎.
2fdad55 to
c69970a
Compare
|
Rebased onto integration/aurora e636747 after #1167 merged: the 23 carried #1167/#945 commits are dropped and the PR is now just the apple,manual-availability binding and the J716s N1 training DT. The built DTB differs from the pre-rebase one only by the maintainer changes (bootargs, MTP compatible) and the SMC GPIO node is now named |
RyanTheTide
left a comment
RyanTheTide
left a comment
There was a problem hiding this comment.
d8e0d7d (author edimoldovan) has no Signed-off-by from its author. The J716s DT also uses PCIe reg-names (phy-shared, phy-common, axi2af, pcieclkgen, phy-window, phy-ip) that the apple,pcie binding on integration/aurora does not list, so dt-checks fails until #1182's binding change is in, and t6050-j716s.dts now conflicts with integration/aurora.
…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
c69970a to
ae9d6a1
Compare
|
Updated (head ae9d6a1). The branch is now #1182's ten commits plus the DT commit, so it shows those until #1182 is in.
On the Codex P1: |
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
pcie-apple trains a T6050 port the loader left down if the root port
names its endpoint's power line, and needs for it the shared PHY, the
PHY common block and the windows of the host tunable lists. Describe
them for the J716s, from its ADT (the same windows as the J714c's):
- pcie0: phy-shared (reg[3]), phy-common (reg[2] + 0x4000) and the
axi2af (reg[5]), pcieclkgen (reg[6]), phy-window (reg[2]) and phy-ip
(reg[4]) windows; the lists themselves differ between machines and
come from the loader;
- port00: pwren-gpios, SMC GPIO gP13, the N1's power
(pci-bridge0 function-reset = pKW4("gP13", 0x00800000));
- the SMC's first GPIO bank (gP keys), which gP13 is on;
- dart_apcie0 waits for its root complex (apple,manual-availability):
enabled with the port down, it is probed only once pcie-apple has the
port up;
- pcie0 lists dart_apcie0 in post-init-providers, so that
fw_devlink.strict=1 does not make the host wait for the DART that waits
for the host (the iommu-map supplier link is then ignored);
- the power of the port: iBoot leaves APCIE_SYS_GP, APCIE_PHY_SW and
APHY_GP_AUS5 in automatic power management with the block gated, and
the train needs them on and staying on, as m1n1 raises them. They and
their parents are always on, with auto-PM off on the three: the
port's link is kept through suspend-to-idle, so they must not follow
the host's sleep.
port00 and dart_apcie0 stay disabled in the DT: the loader enables them
when the port is up or the kernel can train it.
Signed-off-by: Rolando Trevino <rolotrealanis@gmail.com>
Assisted-by: LLM Claude
ae9d6a1 to
a144e4c
Compare
DjDeveloperr
left a comment
DjDeveloperr
left a comment
There was a problem hiding this comment.
Codex (GPT-6.1 Sol), reviewing on @DjDeveloperr’s behalf.
No blocking findings after the dependency refresh/source comparison. Prerequisites1182 and1167 are merged; most carried driver/binding files already match current integration, and newer unrelated DART/PCIe work is retained. The remaining per-board root-port/post-init supplier description is bounded to J716s. Exact-head DT and host checks are green. Existing hardware reports establish N1 link training; the later supplier-order addition is compile/schema checked rather than a new strict-fw_devlink boot claim. Treat that boot as a rollout follow-up and preserve current supplier validation.
a75e4d2
into
aurora-silicon:integration/aurora
The device-tree half of the kernel-side N1 training for the J716s (MacBook Pro 16-inch, M5 Pro), stacked on #1167 (the J716s board device tree): edimoldovan's DART binding for
apple,manual-availabilityand the J716s DT changes that let the kernel train apcie0 port 0 itself (shared and common PHY regions, the four tunable windows, the N1 power line as an SMC GPIO, the first SMC GPIO bank,dart_apcie0marked manually available, and the GP power chain). The driver half is #1182 (on #937).Tested on the J716s together with the driver half (same patches rebased onto our series): the kernel trains the port cold and after a warm restart with Wi-Fi in use, all three N1 functions enumerate, Wi-Fi and Bluetooth come up; the loader-trained path still works. Checks: compile-check per commit, dtbs_check on t6050-j716s (rc 0), checkpatch clean apart from the missing author Signed-off-by on the carried DART binding commit.