mtime_o could be instable

[GTrannoy]

You have export the mtime_o to top instance and I thank you for that ! It helps me a lot anf avoid to add a timer in my project.

But I observed instabilities, probably because 'time_o' in the file "neorv32_mtime.vhd" is not in a synchronous process...
Although, I read it in a synchronous process. It is always when the msb words 'mtime_hi' updates that it reads a bad value.

To validate a good reading whithout changing something in the core, I added a process to check mtime_o change only by '1' between two clock ticks. But it is a little messy to add this process to validate the ntime output.

I don'k know why it is this instability... Maybe putting the line 167 (' time_o <= mtime_hi & mtime_lo(31 downto 00); ') in a synchronous process ?

[stnolting]

What do you mean with "instabilities"?

The 64-bit MTIME counter is split in two 32-bit counter (timing issues), so there might be an inconsistency when the lower word overflows into the higher word as the carry propagation requires an additional cycle.

Let me illustrate that:

MTIME
0x0000.0000_ffff.fffe
0x0000.0000_ffff.ffff
0x0000.0000_0000.0000 <- inconsistency
0x0000.0001_0000.0001

For the CPU that is no deal at all as it reads/writes MTIME only in chunks of 32-bit.

Is that what you mean with "instability"?

[stnolting]

The 64-bit MTIME counter is split in two 32-bit counter (timing issues), so there might be an inconsistency when the lower word overflows into the higher word as the carry propagation requires an additional cycle.

This seems to be a real "bug" 😅
Not in terms of the CPU itself, but external components that rely on mtime_o might have troubles with this inconsistency.

But I observed instabilities, probably because 'time_o' in the file "neorv32_mtime.vhd" is not in a synchronous process...
Although, I read it in a synchronous process. It is always when the msb words 'mtime_hi' updates that it reads a bad value.

You were absolutely right. I am sorry, that I did not see that at first. I will add a synchronization delay to mtime_o to fix that 👍

[stnolting]

Should be fixed now 😉🚀

[GTrannoy]

Yes, that is this inconsistency which disturbs other modules in my project.
Thank you if you have fixed it !

[GTrannoy]

The problem of inconsistency is not corrected.

I have made some tests with testbench, and the most simple is just to put the line with 'mtime_lo_ovfl' outside the process, with the mtime_lo increment:

  -- mtime.time_LO increment --
  mtime_lo_nxt <= std_ulogic_vector(unsigned('0' & mtime_lo) + 1);
  mtime_lo_ovfl(0) <= mtime_lo_nxt(32); -- overflow (carry)

And with this modification, 'mtime_lo_ovfl' is updated before the same clock cycle which updates 'mtime_lo' and 'mtime_hi'.

[stnolting]

And with this modification, 'mtime_lo_ovfl' is updated before the same clock cycle which updates 'mtime_lo' and 'mtime_hi'.

Right, by this the LOW and HIGH words of the mtime counter are always "sync". The problem here is that this will introduce a 64-bit carry chain, which will reduce maximum clock frequency. That is why I split that up into two 32-bit carry chains (one for the HIGH word, one for the LOW word) that are coupled via the registered mtime_lo_nxt(32).

I have made some tests with testbench,

Could you share or specify your simulation scenario?

I have also done some simulation. I have set mtime to 0x00000000.FFFFFF00 to generate a carry from the LOW word to the HIGH word after 256 cycles. The waveform below shows this carry. mtime_lo and mtime_hi are out of sync as there is one cycle "offset" due to the registered carry. However, the processor's mtime_o signal uses a simple register buffer for the lower 32-bit to re-sync HIGH and LOW words in that signal (white signal in waveform).

There is no inconsistency in mtime_o. At least I cannot find any. 🤔

[GTrannoy]

I have also done some simulation. I have set mtime to 0x00000000.FFFFFF00 to generate a carry from the LOW word to the HIGH word after 256 cycles.

This is strange, I have done the same and I obtain with ModelSim the datagram which shows the inconsistency:

The problem here is that this will introduce a 64-bit carry chain, which will reduce maximum clock frequency.

Ok. I didn't take care of that when I suggested the modification.
Anyway, with modification I wrote before, I have now this datagram with correct transition:

Moreover, Libero (IDE for Microsemi fpga) can synthesize and I don't have the error again on my real fpga.

Could you share or specify your simulation scenario?

This is simple, I have just specify the command 'neorv32_mtime_set_time( 0x00000000FFFFFF00ULL);' before infinite loop in 'main'. And after I have only somme reads/writes continuously in the loop.

I don't know why we have not the same behavior...

[umarcor]

We've seen mismatches between GHDL and ModelSim due to delta cycle ordering issues: VUnit/vunit#692. Might this be the case here?

[stnolting]

Oh wait, I think this is just a misunderstanding 😄

I think you are checking time_o in your testbench:

neorv32/rtl/core/neorv32_mtime.vhd

Lines 55 to 56 in 74ee026

	-- time output for CPU --
	time_o : out std_ulogic_vector(63 downto 0); -- current system time

time_o is the output of the MTIME module and this output does not provide a synchronization for LOW and HIGH words since this is irrelevant for the CPU.

I was talking about the mtime_o output of the whole processor:

neorv32/rtl/core/neorv32_top.vhd

Lines 187 to 189 in 74ee026

	-- System time --
	mtime_i : in std_ulogic_vector(63 downto 0) := (others => '0'); -- current system time from ext. MTIME (if IO_MTIME_EN = false)
	mtime_o : out std_ulogic_vector(63 downto 0); -- current system time from int. MTIME (if IO_MTIME_EN = true)

mtime_o provides the "low-word synchronization" that is implemented in neorv32_top.vhd (not in the MTIME module itself):

neorv32/rtl/core/neorv32_top.vhd

Lines 944 to 961 in 74ee026

	-- system time output LO --
	mtime_sync: process(clk_i)
	begin
	if rising_edge(clk_i) then
	-- buffer low word one clock cycle to compensate for MTIME's 1-cycle delay
	-- when overflowing from low-word to high-word -> only relevant for processor-external devices
	-- processor-internal devices (= the CPU) do not care about this delay offset as 64-bit MTIME.TIME
	-- cannot be accessed within a single cycle
	if (IO_MTIME_EN = true) then
	mtime_o(31 downto 0) <= mtime_time(31 downto 0);
	else
	mtime_o(31 downto 0) <= (others => '0');
	end if;
	end if;
	end process mtime_sync;
	-- system time output HI --
	mtime_o(63 downto 32) <= mtime_time(63 downto 32) when (IO_MTIME_EN = true) else (others => '0');

[GTrannoy]

Yes !!! I understand what happens !
You are right, I was checking time_o.

And I have the problem because I updated only neorv32_mtime.vhd but not the top instance.
And I just saw you have added the process mtime_sync to handle with this problem...

I am sorry, it is my mistake ! 😕

[stnolting]

I am sorry, it is my mistake ! 😕

Absolutely no problem.
I'm happy that you setup works now! 🎉 👍