Fixes for AXI4 MMIO and FBus ports #618
Conversation
* Fixes bug with AXI4 MMIO ports not being generated properly due to IOBinders issue. Additionally adds IOCells to AXI4 ports so that they appear in ChipTop * Change IOBinders to also require passing p: Parameters to child functions. Serialization of type targets via ClassTags fails for compound types, so we cannot use `BaseSubsystem with HasSomeTrait` as the type target in OverrideIOBinders.
There was a problem hiding this comment.
I'm a little be concerned that there could be instances where the top-level parameters instance may not match that used in the mixin, which is probably what the user wants most cases. It's unfortunate the self-typing in the mixins is insufficient for the type system here.
That said, I don't see any other way to fix this without going into RC though.
There was a problem hiding this comment.
I won't block this PR, but I share the concern about parameter mismatches and being overly verbose/repetitive. There seem to only be a few IOBinders that need this- can you fix this with types somehow? e.g.
type chipyard.CanHaveSlaveAXI4Port = freechips.rocketchip.subsystem.CanHaveSlaveAXI4Port with BaseSubsystem
?
| } | ||
| def axi4(io: Seq[AXI4Bundle], node: AXI4MasterNode, name: String): Seq[(AXI4Bundle, AXI4EdgeParameters, Seq[IOCell])] = { | ||
| io.zip(node.out).zipWithIndex.map{ case ((mem_axi4, (_, edge)), i) => { | ||
| //val (port, ios) = IOCell.generateIOFromSignal(mem_axi4, Some(s"iocell_${name}_axi4_master_${i}")) |
There was a problem hiding this comment.
I would like this to be fixed, but I also understand that I'm in the critical path for that :). I'll take a look, but I won't block this PR because of this.
jerryz123
force-pushed
the
mmio_fix
branch
3 times, most recently
from
0e41fdf
to
e4edd4c
Compare
There was a problem hiding this comment.
So, I was actually thinking you would call GetSystemParameters() where you need it, rather than forcing every IOBinder to use it. This setup prevents you from using IOBinders on a system that is not a LazyModule (which we probably won't need to do soon/ever, tbh). It also adds an additional parameter to the IOBinder function, which IMO is still not ideal.
jerryz123
force-pushed
the
mmio_fix
branch
2 times, most recently
from
92a2828
to
b363dc4
Compare
jerryz123
force-pushed
the
mmio_fix
branch
3 times, most recently
from
40a73e7
to
db5a8f1
Compare
Related issue: #616
Type of change: bug fix | new feature
Impact: rtl change
IOBinders issue. Additionally adds IOCells to AXI4 ports so that they
appear in ChipTop
to child functions. Serialization of type targets via ClassTags fails
for compound types, so we cannot use
BaseSubsystem with HasSomeTraitas the type target in OverrideIOBinders.