YosysHQ / yosys

Yosys Open SYnthesis Suite
https://yosyshq.net/yosys/
ISC License
3.3k stars 860 forks source link

Inout can't be read with constant value #4370

Open alchitry opened 2 months ago

alchitry commented 2 months ago

Version

Yosys 0.40+45 (git sha1 dd2195543, g++ 13.2.1 -march=x86-64 -mtune=generic -O2 -fstack-protector-strong -fPIC -Os)

On which OS did this happen?

Linux

Reproduction Steps

Here's a minimal example.

module MI_alchitryTop (
    output reg [7:0] P_led,
    inout [4:0] P_button
  );

  reg [4:0] IO_P_button;
  assign P_button = IO_P_button;

  always @* begin
    IO_P_button = 5'bzzzzz;
    P_led = P_button;
  end

endmodule

I'm targeting the iCE40HX8K-CB132

Expected Behavior

The P_button signal to be read and output on P_led.

Actual Behavior

The P_button signal is treated as a constant and the LEDs never turn on.

KrystalDelusion commented 2 months ago

I don't have any experience with using inout ports or tristate logic, but having a quick play with the show command and stepping through the synth command (cough cough) it looks like any semblance of tristate logic is being dropped as soon as it reaches proc. Looking at a few examples online for tristate logic it seems like you need to assign it with conditional logic, i.e. assign P_button = P_button ? P_button : 'bz; for it to correctly identify the tristate logic, rather than simply writing z's because then it will optimise to always read back z's (which, again you can see with show). This is probably also why your other issue, #4371, works with IO_P_button = D_flip_q ? 5'bzzzzz : 5'h0;.

alchitry commented 1 month ago

For what it's worth, this works correctly in both iCEcube2 and Vivado.

I understand this case is kind of pointless since the inout is always z but it should act just like an input at that point and have its value driven externally.

KrystalDelusion commented 1 month ago

I agree, but given that read_verilog does report back Warning: Yosys has only limited support for tri-state logic at the moment. for the relevant line, I think this is more of a feature request than a bug.