Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Incremental builds may reuse GEM3 objects when switching the GEM selector without a clean rebuild.
Review effort: Lite
Findings: None
What changed in this PR
Updates ZynqMP GEM support for correct AArch64 data width and selectable GEM instances.
Changes:
- Enables 64-bit AMBA mode for AArch64 builds.
- Selects GEM address, IRQ, reset, and clock settings by index.
- Supports externally managed SGMII reference clocks.
| File | Description |
|---|---|
src/port/amd/common/gem_core.c |
Fixes AArch64 GEM data-width configuration. |
src/port/amd/boards/zcu102/board.h |
Adds selectable GEM instance mappings. |
src/port/amd/boards/zcu102/board_gem.c |
Applies selected clock/reset settings and external-clock handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
dgarske
force-pushed
the
zynqmp_gem_select
branch
from
September 23, 2026 20:07
049fe59 to
174c110
Compare
dgarske
force-pushed
the
zynqmp_gem_select
branch
from
September 23, 2026 23:00
174c110 to
7a25ba8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes for the AMD Xilinx ZynqMP GEM port, all of which bite once it is used outside the ZCU102's default arrangement.
64-bit AMBA data width.
NWCFG_DWIDTH_64was set under#ifdef XILINX_AARCH64, a define only the zcu102 and versal Makefiles add. Built any other way the MAC runs a 32-bit datapath on a 64-bit master and never transmits: descriptors are armed,TSR.TXGOasserts,txcntstays zero, while receive works normally. Now keyed off__aarch64__as well. ARMv7 is unchanged, where the bit is inert.Selecting the controller. GEM3 was hardcoded across four values that must agree: base address, interrupt,
RST_LPD_IOU0reset bit andCRL_APBclock register.-DZYNQMP_GEM_INDEX=1moves all four together; the default stays 3, and an out-of-range index fails the build. Boards that share one MDIO bus across controllers can keep management where the pins are with-DZYNQMP_GEM_MDIO_INDEX, and pin the address with-DGEM_PHY_ADDRrather than relying on a scan that takes whichever PHY answers first. Where platform firmware has already set a controller up,-DZYNQMP_GEM_EXT_REF_CLKand-DZYNQMP_GEM_NO_RESETstop the driver reclocking or resetting it. A stamp file forces a rebuild when any of this changes, since make cannot seeCFLAGS_EXTRAas a dependency.Foreign PHYs.
dp83867_init()soft-reset the PHY and wrote TI registers whatever it found, then read speed and duplex from TI'sPHYSTS. On another vendor's part the reset discards what the platform configured, the indirect writes land in its MMD space, and the speed decode is meaningless: one board came up with the MAC at 10 Mbps half duplex against a gigabit link and passed no traffic. A PHY that is not a DP83867 is now left as its own init left it, only autonegotiation is restarted, and speed is resolved from the clause-22 registers every 802.3 PHY implements.Diagnostics.
-DDEBUG_PHYnow reports the PHY's state on entry, before the driver writes anything:BMCRwithPDOWNandISOLATEdecoded, the advertisement and partner registers, and every responding MDIO address with the pinned one marked. The autonegotiation and link waits are settable with-DGEM_PHY_ANEG_TIMEOUT_MSand-DGEM_PHY_LINK_TIMEOUT_MS.SGMII, opt-in and untested.
-DZYNQMP_GEM_SGMIIselects the internal PCS (NWCFG.PCSSELand the SGMII mode bit) and runs its clause-37 negotiation with the PHY, which is separate from the PHY's copper negotiation with the link partner. It implies-DZYNQMP_GEM_EXT_REF_CLK, since on SGMII the reference comes from the PS-GTR serdes and a speed downshift must not reprogramCRL_APB. There is no SGMII board here to exercise it on, so it is off by default and should be treated as a starting point rather than a working configuration: the serdes must already be up, and a board may need lane or PCS setup beyond this.Test status
boards/zcu102builds for the default, each GEM index, the MDIO split, a pinned PHY address, both clock and reset options, SGMII,DEBUG_PHY, and extended timeouts; out-of-range index, PHY address and timeout values all fail the build as intended.boards/zynq7000is unchanged and still clean for ARMv7.Hardware, on a ZCU102 with a DP83867 on GEM3: the data-width fix took the board from transmitting nothing to a peer answering in 1 ms, and after the PHY changes the default path still detects the DP83867 and negotiates 1000 Mbps full duplex. Forcing a GEM with no PHY reproduces
no PHY responding on MDIOas expected. The foreign-PHY, MDIO-split and SGMII paths have no hardware here and are not exercised.