Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use of ~LONGESTPERIOD prevents absolute time calculation in Clash simulation #2455

Closed
martijnbastiaan opened this issue Apr 23, 2023 · 9 comments
Labels

Comments

@martijnbastiaan
Copy link
Member

Xilinx's primitive models use a global set/reset (GSR) that is used to implement initial values. This reset signal is (by default) defined as asserted for 100 ns. Therefore, to faithfully replicate the VHDL/Verilog model, Clash simulation will have to know the absolute simulation time. Unfortunately, Clash's clock generation primitives wait for ~LONGESTPERIOD before it makes clock tick, a value only known at RTL compile time.


Encountered in: #2452. Waveform:

Screenshot from 2023-04-22 20-36-44

Note that the first three cycles (0000, 0001, 0002) are ignored by the pipeline because GSR is asserted.


@christiaanb Do you remember the rationale behind ~LONGESTPERIOD? Could we simply deprecate it / set it to a static value?

@christiaanb
Copy link
Member

It was done to fix issue #2001 so a static value will always be wrong if we have clock period that is longer than said static value.

So perhaps we can add a static value on top of ~LONGESTPERIOD.

@christiaanb
Copy link
Member

Why do we even model GSR in #2452? According to https://docs.xilinx.com/r/en-US/ug900-vivado-logic-simulation/Global-Set-and-Reset-Net you only want to deal with explicit GSR in post-synt/post-impl simulation.

@christiaanb
Copy link
Member

Perhaps we want to change:

to: lauch_simulation -mode behavioral

See also: https://docs.xilinx.com/r/2021.2-English/ug900-vivado-logic-simulation/Behavioral-Simulation

@DigitalBrains1
Copy link
Member

DigitalBrains1 commented Apr 23, 2023

Huh, I never realised we were not running a behavioural simulation in Xilinx 😅

I agree, let's switch to behavioural simulation. Perhaps it will even be less hideously slow.

@martijnbastiaan
Copy link
Member Author

Why do we even model GSR in #2452?

It's what the Xilinx's XPM_CDC_GRAY model does (unless #ifdef ONESPIN). The GSR is defined by verunilibs's glbl.v. So if we want a faithful correspondence between Verilog/VHDL simulation and Clash simulation we'll need to replicate what the GSR does. (It's not a hard requirement for me for that PR, but not be able to query the absolute time during Clash simulation does seem like a fundamental shortcoming to me.)

Perhaps we want to change:

+

Huh, I never realised we were not running a behavioural simulation in Xilinx

We do :). behavioral is the default.

@martijnbastiaan
Copy link
Member Author

So perhaps we can add a static value on top of ~LONGESTPERIOD.

Adding 100 ns would entirely sidestep my issues.

@christiaanb
Copy link
Member

@martijnbastiaan where in the docs does it say that behavioral is the default? https://docs.xilinx.com/r/2021.2-English/ug900-vivado-logic-simulation/Behavioral-Simulation seems to suggest otherwise.

@martijnbastiaan
Copy link
Member Author

martijnbastiaan commented Apr 24, 2023

UG835 on launch_simulation:

Screenshot 2023-04-24 at 11-10-47 ug835-vivado-tcl-commands pdf • Viewer • AMD Adaptive Computing Documentation Portal

Running with launch_simulation -mode behavioral gives me:

Screenshot from 2023-04-24 11-16-10

GSR would be the global reset, glblGSR_xpmcdc is the reset line in XPM_CDC_GRAY (copied from GSR).

Edit: which is to say, I believe the GSR is always modeled / taken into account in XPM_CDC_GRAY.

@christiaanb
Copy link
Member

We've released v1.8.0, which includes a fix for this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants