mystorm-org / BlackIce-II

Software, Firmware and documentation for the myStorm BlackIce-II board
68 stars 8 forks source link

"assign my_signal = 1'bz;" doesn't do what you think it does. #14

Open tomverbeure opened 6 years ago

tomverbeure commented 6 years ago

See the Yosys issue that I just filed: https://github.com/YosysHQ/yosys/issues/511

assign my_signal = 1'bz; results in an active low output driver.

All current examples are using this for all unused PMOD and other signals.

As a result, a bunch of IOs on the board may be in short-circuit state.

The best example of this is PMOD[1]/greset: this signal is always driven by the CH340G RTS# signal, and it drives a value of 1 (at least when when USB2 is not plugged in). In all examples, PMOD[1] = 1'bz, so it drives a low.

Apparently, the FPGA is a stronger driver than the CH340G on my board, because otherwise no example would work with greset driving a 1.

Tom

Note: when I fix the issue by not assigning anything to PMOD1, the board is indeed in reset at all times, except when I do "cat /dev/ttyUSB0" or "echo > /dev/ttyUSB0". But as soon as I cancel the 'cat' operation or as soon as the echo is completed, it goes back into reset.

tomverbeure commented 6 years ago

Since constant assignments of 1'bz simply don't work, and actually do active harm, they shouldn't be done at all.

What also doesn't work is to keep the output ports but not assign them: that also results in a hard drive to 0. As can be seen in this post-placement result from Arachne:

  SB_IO #(
    .PIN_TYPE(6'b011001)
  ) $inst0 (
    .PACKAGE_PIN(B1),
    .D_OUT_0($undef)
  );

Also not a good solution: do nothing. Having IOs constantly in contention is just not a good idea.

Here are possible solutions:

folknology commented 6 years ago

I like your fix contentions for the reset (the assign my_signal = 1'bz; is a PITA in yosys) it is a great idea to sort this out generically for all examples. Perhaps if we use -no-warn on the set_ios ( #9 ) we could remove all other none used io's from the examples?

folknology commented 6 years ago

Maybe we just use 'inout' rather than 'out' in all cases of tri-stating

folknology commented 6 years ago

I'm having trouble checking this, just updated yosy after some installation issues on my laptop (I'm remote at the mo), but tri-stating is complicated with yosys - This is an interesting thread around the matter https://github.com/cseed/arachne-pnr/issues/24

folknology commented 6 years ago

I'm playing around with some of the examples to find a better expression for these

TimRudy commented 3 years ago

Is this resolved by fix of https://github.com/YosysHQ/yosys/issues/1841, which was generic, in iopadmap? Just checking in 2021.

tomverbeure commented 3 years ago

I just reran this as follows:

read_verilog test.v
hierarchy
synth_ice40
write_blif test.blif
write_verilog test.out.v

And the output was this:

(* top =  1  *)
(* src = "test.v:1.1-13.10" *)
module tritest(my_tri_out, my_tri_inout, my_in0, my_in1);
  (* src = "test.v:5.15-5.21" *)
  input my_in0;
  (* src = "test.v:6.15-6.21" *)
  input my_in1;
  (* src = "test.v:3.15-3.27" *)
  inout my_tri_inout;
  (* src = "test.v:2.16-2.26" *)
  output my_tri_out;
  (* src = "test.v:11.31-11.53" *)
  \$_TBUF_  \my_tri_inout_$_TBUF__Y  (
    .A(my_in1),
    .E(my_in0),
    .Y(my_tri_inout)
  );
  assign my_tri_out = 1'hz;
endmodule

The my_tri_out = 1'hz; is still there.

In my own designs, I pretty much always instantiate IO buffers when I want anything other than pure input or output, so this bug is not an issue for me anymore.

Tom