move common sim infra to chipflow-lib #122
Conversation
|
|
robtaylor
force-pushed
the
sim-common
branch
7 times, most recently
from
ab61ad2 to
73c0bd6
Compare
robtaylor
force-pushed
the
sim-common
branch
13 times, most recently
from
778f36c to
5d9304e
Compare
chipflow_lib/steps/sim.py
Outdated
| for n, t in top.items(): | ||
| setattr(m.submodules, n, t) | ||
|
|
||
| for component, iface in self._platform._pinlock.port_map.items(): |
There was a problem hiding this comment.
could some of this code be shared with SiliconTop?
There was a problem hiding this comment.
I don't know - i didn't originate this so I don't really understand the difference between the code here and the code in SiliconTop.
Leave for now? maybe you understand better and can do a pr after?
There was a problem hiding this comment.
it's not my code either - I don't know where it came from originally but thought all the IOSignature stuff was yours - but that seems fair enough for now
There was a problem hiding this comment.
I guess i must have written it but its all dropped from my mind. i'll have to look again.. a bit worried that they should be the same and that the silicon case is missing something important...
chipflow_lib/steps/sim.py
Outdated
| else getattr(iface, name)) | ||
| port = self._platform._ports[port.port_name] | ||
| if hasattr(wire, 'i'): | ||
| m.d.comb += wire.i.eq(port.i) |
There was a problem hiding this comment.
Inverted ports should also be inverted here
There was a problem hiding this comment.
I don't understand this either - it runs... what does inverting here do?
There was a problem hiding this comment.
it works at the moment because we don't use invert anywhere (because we didn't support it up to now). but with this now inverted ports would behave wrongly in sim
There was a problem hiding this comment.
ahha, i see thank you
chipflow_lib/platforms/sim.py
Outdated
|
|
||
| for reset, name in self._config["chipflow"]["resets"].items(): | ||
| port_data = pinlock.package.resets[name] | ||
| port = io.SimulationPort(io.Direction.Input, port_data.width, invert=port.invert, name=f"clock-{name}", ) |
There was a problem hiding this comment.
this should be reset- not clock-
chipflow_lib/platforms/sim.py
Outdated
| raise ChipFlowError(f"Unable to find clock {name} in pinlock") | ||
|
|
||
| port_data = pinlock.package.clocks[name] | ||
| port = io.SimulationPort(io.Direction.Input, port_data.width, invert=True, name=f"clock-{name}") |
There was a problem hiding this comment.
why is invert=True here? clocks wouldn't normally be inverted
chipflow_lib/platforms/sim.py
Outdated
|
|
||
| for reset, name in self._config["chipflow"]["resets"].items(): | ||
| port_data = pinlock.package.resets[name] | ||
| port = io.SimulationPort(io.Direction.Input, port_data.width, invert=port.invert, name=f"clock-{name}", ) |
There was a problem hiding this comment.
what is port.invert supposed to refer to? port isn't yet set at the time you use it, so it will actually be referring to the last clock or reset....
There was a problem hiding this comment.
random mess, i think, no real way to say if resets are inverted or not, so this should either be always not inverted or always inverted...
chipflow_lib/steps/__init__.py
Outdated
| m.d.comb += wire.i.eq(port.i ^ inv_mask) | ||
| for d in ['o', 'oe']: | ||
| if hasattr(wire, d): | ||
| m.d.comb += getattr(port, d).eq(getattr(wire, d) ^ inv_mask) |
There was a problem hiding this comment.
oe is never inverted regardless of port.invert, invert only refers to the input/output data
chipflow_lib/steps/__init__.py
Outdated
| if hasattr(wire, 'o'): | ||
| m.d.comb += port.o.eq(wire.o)^ inv_mask | ||
| if hasattr(wire, 'oe'): | ||
| m.d.comb += port.oe.eq(wire.oe)^ inv_mask |
There was a problem hiding this comment.
seems a previous comment was missed - oe should never be inverted
|
one last comment left - then this should be good to go |
|
Oh weird, sure I’d fixed that! Will fix and land it :)
…On Tue, 8 Jul 2025 at 10:38, myrtle ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In chipflow_lib/steps/__init__.py
<#122 (comment)>
:
> + for iface_name, member, in iface.items():
+ for name, port in member.items():
+ iface = getattr(top[component], iface_name)
+ wire = (iface if isinstance(iface.signature, IOSignature)
+ else getattr(iface, name))
+ if port.invert:
+ inv_mask = sum(inv << bit for bit, inv in enumerate(port.invert))
+ else:
+ inv_mask = 0
+ port = platform._ports[port.port_name]
+ if hasattr(wire, 'i'):
+ m.d.comb += wire.i.eq(port.i ^ inv_mask)
+ if hasattr(wire, 'o'):
+ m.d.comb += port.o.eq(wire.o)^ inv_mask
+ if hasattr(wire, 'oe'):
+ m.d.comb += port.oe.eq(wire.oe)^ inv_mask
seems a previous comment was missed - oe should never be inverted
—
Reply to this email directly, view it on GitHub
<#122 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB4O4GTW7KJRP56DEKGULT3HOGRRAVCNFSM6AAAAACAA5MHSWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDSOJWHAYDAMJZGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
No description provided.