Problems with 'tperiod_Clk'
Paebbels opened this issue · 2 comments
Thanks for yesterdays live debugging session.
We found our mistake in the usage of Axi4Manager and Axi4Memory.
The frequency of the OSVVM verification components was not matching the period of our testbench clocks.
We have 2 suggestions to prevent others from doing the same mistake:
- Don't set a default value to
tperiod_Clk
, so users are forced to map a clock period. - Measure the clock period in the model.
=> We would prefer this method.
How can the latter be implemented in a save and reliable way?
- Measure the clock period on
Clk
and save it in a local variable/signal. - When reset is done (
rising_edge(nReset)
, assigntperiod_Clk
.
What does this method achieve?
- No need for a generic / no dependency to the parent instance.
- Clock period can be changed in the testbench, while reset is high or retriggered.
- Anyhow, clock must be stable before reset is released and it can't change frequency afterwards unless reset is asserted again.
- When reset is deasserted,
tperiod_Clk
can be used immediately.
Sketch of the solution:
architecture a of e is
signal tperiod_Clk : time := -1 ns;
begin
process(Clk, nReset)
variable LastRise : time := 0 ns;
variable Period : time := -1 ns;
begin
if rising_edge(nReset) then
tperiod_Clk <= Period;
end if;
if rising_edge(Clk) then
Period := now - LastRise;
LastRise := now;
end if;
end process;
-- ...
end architecture;
Note:
- negative times are intended, so simulation fails on uninitialized / unmeasured clocks.
/cc @stefanunrein
I also think getting rid of the tperiod_Clk
generic would be favorable. I have some alternative suggestions.
Looking at DoAxiReadyHandshake
,
AXI4/common/src/Axi4CommonPkg.vhd
Line 136 in 3e52ab8
we could change the code to something like
if ReadyBeforeValid then
WaitForClock(Clk, ReceiveReadyDelayCycles);
Ready <= transport '1' after tpd_Clk_Ready ;
end if ;
However, I am not sure if it is OK to make procedure DoAxiReadyHandshake
explicitly wait here. It's probably OK because without a Ready
signal nothing else happens on the bus anyway?
Or maybe something like this
Ready <= transport '1' after Clk'last_event * 2 + tpd_Clk_Ready;
assuming Clk has a 50% duty cycle. Admittedly, I don't know if the second version would actually work, I have never used this attribute before.
Alternatively, the clock-period could also be provided by means of SetAxiStreamOptions(...)
.
What's the reason for this proposed change?
We have a DUT which has an AXI Stream as output. However, the DUT interfaces a high-speed serial link which operates on three different speeds (using auto-negotiation). Depending on the speed, also the AXI output's clock frequency changes during operation. Using a generic for providing the clock-period thus is not working in such a scenario.
I can prepare a PR in case you agree to one of the proposals.
@riedel-ferringer
I will take care of it. This sort of update is probably needed in a couple of places in the VC also.