VexRiscvAxi4LinuxPlicClint not respecting the AXI protocol

[PedroAntunes178]

Hi @Dolu1990,

First and foremost, congratulations on the excellent work in VexRiscv and NaxRiscv.

I've encountered an issue with the VexRiscvAxi4LinuxPlicClint.v file generated by VexRiscv. It appears to not comply with the AXI protocol, specifically related to the WVALID signal. According to the AXI documentation:

When asserted, WVALID must remain asserted until the rising clock edge after the slave asserts WREADY.

As you can see from the image below, the dBusAxi_wvalid is brought down to 0 before the dBusAxi_wready is asserted. However, as shown in the screenshot below, the dBusAxi_wvalid signal is de-asserted before dBusAxi_wready is asserted.

Upon analyzing the CPU's internal signals, I noticed that the valid signal goes down to 0 because dbus_axi_b_fire (bvalid & bready) is asserted and the signal when_Utils_l659, which I believe should be 1, has a value of 0 due to dBus_cmd_fire being 0. dBus_cmd_fire seems to only be asserted when both dBusAxi_wready and dBusAxi_wvalid are 1.

I would like to use this CPU on the SoC I am developing. Therefor this issue is of upmost importance for me. How could I help to solve this issue? I have very limited knowledge in SpinalHDL.

[Dolu1990]

Hi,

Yes that's wrong.
Which version of VexRiscv are you using ? (git hash)
Also, can you send me your wave file + VexRiscvAxi4LinuxPlicClint.v you are using ?

Either the issue is on the VexRiscv axi convertion, either it is on the AXI itself returning too many B transactions

Thanks :)

[PedroAntunes178]

Hello,

Thank you for your prompt response. I wanted to provide you with the information you requested along with the relevant files.

The VexRiscv version I'm using is identified by this git hash: 5ef1bc7. You can access the VexRiscvAxi4LinuxPlicClint.v file through this GitHub link: VexRiscvAxi4LinuxPlicClint.v. I've also sent you an email with the VexRiscvAxi4LinuxPlicClint.v file attached, as per your request. Additionally, I've included the wave files you asked for in the same email.

If you need any more information or have further questions, please don't hesitate to reach out. I appreciate your assistance.

Best regards,
Pedro Antunes

[Dolu1990]

Hi,

I think you may have a cursed simulator.

Here is a proof :

  always @(*) begin
    _zz_when_Utils_l687_1 = 1'b0;
    if(dbus_axi_b_fire) begin
      _zz_when_Utils_l687_1 = 1'b1;
    end
  end

=>

I don't understand how can be 'X' while it only depends on dbus_axi_b_fire which is not 'X'
XD

Which simulator / version of simulator are you using ?

[PedroAntunes178]

Hi,

I used "Icarus Verilog version 11.0 (stable)".
I am curently testing with Verilator and Xcelium and they have a different behaviour. I can give more information after analysing the waves.

[jjts]

Since dbus_axi_b_fire never toggled, the always statement never triggered. Could this be it? In Verilog, the reg type must be either initialized by reset or depend on something that does. My 2 cents.

[Dolu1990]

Since dbus_axi_b_fire never toggled, the always statement never triggered.

Yes, probably, maybe it depend on something which is initialized in its own decalaration ? (wire x = 1'b0) ?

[jjts]

wire x = 1'b0 is not good practice either.

This is bound to confound simulation tools. It may be interpreted as a signal permanently assigned to 0. So, if later assigned to anything else, it may result in X depending on drive strength.

Also, this kind of initialization is ignored by synthesis tools.

Always statements must see an edge so they are triggered. We know it works with xcelium from Cadence, which may consider instant 0 a special trigger.

If, above, 'reg' is replaced by signal, and 'always' is replaced by 'assign', works for sure.

What I meant before was a true physical init. For example, the output of a flip-flop after a reset or clk edge. These work for simulation and synthesis.

It should be

wire x;

No static init.

If this signal is always X, it is because it has no meaningful initialization.

Sometimes, I write

wire a = b&c;

This works in every simulator and is equivalent to

wire a;
assign a=b&c;

The latter is a better practice as it avoids the confusion with an initialization.

Sorry for the long message :-)

[PedroAntunes178]

Hi @Dolu1990,

After comparing the waveforms from "Icarus Verilog (IVerilog)" and "Xcelium," we can conclude that the problem is indeed, as @jjts said because the _zz_when_Utils_l687_1 always block is never triggered. Therefore, its value remains 'x' until dbus_axi_b_fire is asserted. This leads to when_Utils_l687 never being asserted, and _zz_dBus_cmd_ready_1 is never equal to '0x001'.

One solution would be to assign a default value to the reg variable, such as reg _zz_when_Utils_l687_1 = 1'b0.

The following change solved the issue, but there may be more problems with untriggered always blocks in "IVerilog":

    reg                 _zz_when_Utils_l687 = 1'b0;   
    reg                 _zz_when_Utils_l687_1 = 1'b0;  
    reg        [2:0]    _zz_dBus_cmd_ready = 3'b000;  
    reg        [2:0]    _zz_dBus_cmd_ready_1 = 3'b000;

I plan to open an issue on https://github.com/steveicarus/iverilog. This could be a problem stemming from the simulator itself.

Best regards,
Pedro Antunes

[jjts]

I think iverilog's interpretation is correct, and no issue needs to be filed unless it is customary to run all always statements at t=0, regardless of any dependencies toggling.

Why do other simulators work? Is it because they run the always statements at t=0? Is this the correct thing to do?

It all comes from the 'reg' type, a common source of confusion in Verilog.

A 'reg' isn't a physical entity. It's not even a resgister, as its name suggests.

A 'reg' is a software variable to help compute the physical wires' values, including those of ports.

The statement
reg a =1'b0;
means a is initialized to 0 at t=0.

A variable keeps its value until assigned otherwise.

Since it toggles at t=0, then the always statement triggers, and simulation works.

However, this can be a bad solution if generalized, leading to circuits that work in simulation but not after synthesized.

Physically, there is only one way to initialize a signal: drive it from a flip-flop that has a reset input. No other way. So, if 'a' is mapped to a wire after synthesis, it will be unitialized, and the circuit will not work.

This is why the 'reg' type should NEVER be initialized in a circuit description. In a testbench, this is acceptable, though.

To conclude, to initialize a signal, drive it from a flip-flop with a reset input or make it depend on some other signal which is either a primary input or an output of a flip-flop that has a reset input.

This text was long but necessary to avoid a bad solution to this problem.

[tomverbeure]

Sorry about the side noise but...

This is why the 'reg' type should NEVER be initialized in a circuit description.

I agree about this for this particular case, but the statement is not generally true for FPGAs, where initialized regs are common, and totally synthesizable for FF. A "reg a = 1;" will result in the corresponding FF to power up with a value of 1 after the bitstream loading has completed. It's a very common construct.

[jjts]

Maybe for some FPGAs, but we find that this project can target ASICs and all sorts of FPGAs. This initialization would not work in an ASIC.

If I want a flip-flop to be initialized at power up, I write

always @(posedge clk, posedge arst)
if (arst)
a <= 1'b1;
else
a <= a_nxt;

Works for all FPGAs and ASICs.

Moreover, the 'reg' may not be a flip-flop. It may simply be a target on a combinational 'always' statement, in which case the initialization is meaningless.

[Dolu1990]

Looking at the netlist itself, i do not the bad pattern we are talking about :

  wire                dbus_axi_b_fire;
  assign dbus_axi_b_fire = (dbus_axi_b_valid && dbus_axi_b_ready);
    always @(*) begin
    _zz_when_Utils_l687_1 = 1'b0;
    if(dbus_axi_b_fire) begin
      _zz_when_Utils_l687_1 = 1'b1;
    end
  end

Which for me is seems fine and safe. Or verilog is just a cursed language ?

both Xcelium and iverilog gives the same result ?

[PedroAntunes178]

Xcelium works as expected, but iverilog presents the problem.

In Xcelium, the _zz_when_Utils_l687_1 has a value of 0 before the bvalid is fired.

[jjts]

How does dbus_axi_b_ready come to be 0 from t=0? If by initialization at declaration than this is the source of the problem. We already checked that dbus_axi_b_valid does change after reset but it is filtered by ANDing with dbus_axi_b_ready=0.

…

On Tue, Sep 12, 2023, 16:52 Dolu1990 ***@***.***> wrote: Looking at the netlist itself, i do not the bad pattern we are talking about : wire dbus_axi_b_fire; assign dbus_axi_b_fire = (dbus_axi_b_valid && dbus_axi_b_ready); always @(*) begin _zz_when_Utils_l687_1 = 1'b0; if(dbus_axi_b_fire) begin _zz_when_Utils_l687_1 = 1'b1; end end Which for me is seems fine and safe. Or verilog is just a cursed language ? both Xcelium and iverilog gives the same result ? — Reply to this email directly, view it on GitHub <#363 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABLUHO23FJP4FMGRCJESCYLX2APFZANCNFSM6AAAAAA4L7ISGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[tomverbeure]

Maybe for some FPGAs, but we find that this project can target ASICs and all sorts of FPGAs. This initialization would not work in an ASIC.

I understand that. But the point is: SpinalHDL should not be limited to ASIC only. If FPGA designers want to create code that doesn't have an explicit reset, that should be possible.

It would make sense to have a SpinalHDL Config option to generate RTL with or without explicit reg initialization.

[PedroAntunes178]

Well, I figured out the problem. I was committing that exact mistake from my side. I had initialized the axi_b_valid register signal with 1'b0, which was causing the issue. The problem was solved by removing the initialization.

In VexRiscvAxi4LinuxPlicClint.v, it appears there are no reg initializations. There is the dbus_axi_b_ready signal, which is constantly assigned to 1'b1, and it might benefit from coming from a flip-flop with a reset value of 1 instead of being a wire. However, I don't have enough knowledge or experience to determine the best practice in this case.

Sorry for any confusion. From my side, I believe this issue could be closed now :)

[Dolu1990]

I don't have enough knowledge or experience to determine the best practice in this case.

I would say we should not let the HDL decide the hardware design for us.
I guess an icarus verilog issue should be opened.

[jjts]

Let's congratulate Icarus Verilog because after trying commercial simulators, iverilog, the free and open-source simulator, was the only one to find the issue.

The issue was real, and it was on our side. As @PedroAntunes178 pointed out, there was an unnecessary

wire a = 1'b0;

This initialization in our design was not synthesizable in general and was bound to create serious problems downstream, besides the fact it was simulating correctly with expensive commercial simulators.

We cannot afford or rely on only synthesizable things for FPGA, and I need to know which FPGA brand will synthesize it and which will not.

We prefer to write tool-agnostic Verilog, although this may be very boring. This is where code generators such as SpinalHDL can be helpful. The tool worries about these details, not the user, who, in this way, is not compromising the design. The user design works equally well with explicit resets or relying upon the startup reset of the FPGA. But my point is: only for that FPGA.

The generator is less useful if the generated HDL only runs on certain FPGAs. The quality of the generated Verilog is much better for SpinalHDL than Migen, which needs code to support tens of different FPGAs and no ASICs. The Migen-produced Verilog is wrong!

Please do not let the same happen to SpinalHDL.

[jjts]

Hence, I don't think there's any issue with iverilog, to be precise.

[Dolu1990]

Please do not let the same happen to SpinalHDL.

Sure :)