rtl: standardize Chisel package + restore PopRoCCRouter IO surface

navigator commented

2026-05-25 17:52:41 -03:00

Owner

Retrospective code review of #63, #64, #65, #66 — 2 findings

These were merged without semantic review (pr-approver only checks CI, no diff inspection — see issue for pr-reviewer role coming up). Walking the diffs reveals:

Finding 1 — Package name split across modules

PR	Module	`package`
#63	`PopRoCCRouter`	`chipyard.pop`
#64	`InterGemminiXbar`	`chipyard.pop`
#65	`PCIeHostBridge`	`pop`
#66	`PopLinkPHY_Behavioral`	`pop`

Different resolver runs picked different namespaces. When FluidPopSoC (#54) eventually instantiates all four, Chisel resolution will fail until they all live in one package.

rtl/build.sbt sets organization := "coop.pop.fluidpop" and is a standalone build (not Chipyard-integrated yet) — so neither name is "correct" in the Chipyard sense; the project just needs to pick one and apply it everywhere.

Proposal: standardise on pop (shorter, matches organization, decouples from Chipyard tree which is read-only upstream). Two-line fix per module.

Finding 2 — PopRoCCRouter has zero IO ports

rtl/src/pop/PopRoCCRouter.scala declares val io = IO(new Bundle {}) — an empty IO bundle. The module elaborates but has no pins at all. While the SPEC §Interface is TBD for widths, the signal categories (RoCC request bundle, response bundle, per-tile fan-out, exception path) are named in the spec and have canonical Chisel types available in freechips.rocketchip.tile (RoCCCommand, RoCCResponse). A skeleton ought to surface those bundles even if widths and decode logic are deferred — otherwise the module is a placeholder, not a skeleton.

Compare to #66 which materialises linkUp = Output(Bool()) with a clear justification, and leaves only the unspecified items as comments.

Proposal: rebase #63's body to pull in Rocket's RoCCCommand / RoCCResponse as the request/response surface, leave per-tile and exception paths as TBD comments.

Acceptance criteria

All four .scala modules (PopRoCCRouter, InterGemminiXbar, PCIeHostBridge, PopLinkPHY_Behavioral) declare package pop.
All four spec files under rtl/tests/<Mod>/<Mod>Spec.scala declare matching package.
sbt -no-colors compile green from rtl/.
PopRoCCRouter IO bundle exposes RoCCCommand (input) and RoCCResponse (output) from freechips.rocketchip.tile, plus a comment block for the still-TBD per-tile fan-out and exception path.
PopRoCCRouterSpec still elaborates green.

Notes

This issue is queue-able for a resolver — small, mechanical, all changes inside rtl/.
Once pr-reviewer role lands (separate work), this exact class of finding gets caught at PR time instead of retrospectively.

## Retrospective code review of #63, #64, #65, #66 — 2 findings These were merged without semantic review (pr-approver only checks CI, no diff inspection — see issue for `pr-reviewer` role coming up). Walking the diffs reveals: ### Finding 1 — Package name split across modules | PR | Module | `package` | |---|---|---| | #63 | `PopRoCCRouter` | `chipyard.pop` | | #64 | `InterGemminiXbar` | `chipyard.pop` | | #65 | `PCIeHostBridge` | `pop` | | #66 | `PopLinkPHY_Behavioral` | `pop` | Different resolver runs picked different namespaces. When `FluidPopSoC` (#54) eventually instantiates all four, Chisel resolution will fail until they all live in one package. `rtl/build.sbt` sets `organization := "coop.pop.fluidpop"` and is a standalone build (not Chipyard-integrated yet) — so neither name is "correct" in the Chipyard sense; the project just needs to **pick one and apply it everywhere**. **Proposal:** standardise on `pop` (shorter, matches `organization`, decouples from Chipyard tree which is read-only upstream). Two-line fix per module. ### Finding 2 — PopRoCCRouter has zero IO ports `rtl/src/pop/PopRoCCRouter.scala` declares `val io = IO(new Bundle {})` — an empty IO bundle. The module elaborates but has no pins at all. While the SPEC §Interface is `TBD` for widths, the *signal categories* (RoCC request bundle, response bundle, per-tile fan-out, exception path) are named in the spec and have canonical Chisel types available in `freechips.rocketchip.tile` (`RoCCCommand`, `RoCCResponse`). A skeleton ought to surface those bundles even if widths and decode logic are deferred — otherwise the module is a placeholder, not a skeleton. Compare to #66 which materialises `linkUp = Output(Bool())` with a clear justification, and leaves only the unspecified items as comments. **Proposal:** rebase #63's body to pull in Rocket's `RoCCCommand` / `RoCCResponse` as the request/response surface, leave per-tile and exception paths as TBD comments. ## Acceptance criteria - [ ] All four `.scala` modules (`PopRoCCRouter`, `InterGemminiXbar`, `PCIeHostBridge`, `PopLinkPHY_Behavioral`) declare `package pop`. - [ ] All four spec files under `rtl/tests/<Mod>/<Mod>Spec.scala` declare matching package. - [ ] `sbt -no-colors compile` green from `rtl/`. - [ ] `PopRoCCRouter` IO bundle exposes `RoCCCommand` (input) and `RoCCResponse` (output) from `freechips.rocketchip.tile`, plus a comment block for the still-TBD per-tile fan-out and exception path. - [ ] `PopRoCCRouterSpec` still elaborates green. ## Notes - This issue is queue-able for a resolver — small, mechanical, all changes inside `rtl/`. - Once `pr-reviewer` role lands (separate work), this exact class of finding gets caught at PR time instead of retrospectively.