Codecov Report

Merging #221 into vendor.altera will not change coverage.
The diff coverage is n/a.

@@              Coverage Diff               @@
##           vendor.altera     #221   +/-   ##
==============================================
  Coverage          82.34%   82.34%           
==============================================
  Files                 33       33           
  Lines               5499     5499           
  Branches            1178     1178           
==============================================
  Hits                4528     4528           
  Misses               837      837           
  Partials             134      134

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdb9937...91e4d7f. Read the comment docs.

So, FAST_{INPUT,OUTPUT}_REGISTER isn't an attribute, but a TCL command, so top.qsf would need to be modified for that. I'm not sure best how to do that.

@ZirconiumX Looks like you can use the altera_attribute attribute in Verilog, something like (* altera_attribute = "-name FAST_INPUT_REGISTER ON" *), see here. But also, it looks like in this case you can use (* useioff *) attribute instead, see here.

Note that you still need to verify whether those will work on alt_*buf instances.

See also this post.

I fixed the mangled branch history, sorry.

Looks good. The next steps would be to add DDR support and then factor out common code so that it's not quite as repetitive. (I think DDR suppor would influence the way you can refactor this.)

Should I put that in a separate PR, or just continue this?

Continue this.

I've been working from UG83105, which also mentions the differential I/O primitives. However, I still need to hunt for what a useful DDR primitive would be, given that using IP blocks seems troublesome.

I think you might be able to generate IP cores with qsys-script and qsys-generate as described here.

I'm unsure if this should advertise SDR support or not at the moment. Thoughts?

It should. I won't merge the complete backend (i.e. the vendor.altera branch, this PR is fine) until SDR and DDR are verified working, anyway.

While writing f24137b a question occurred to me: what's the correct clock to use for an output-enable?

The output clock.

I realised I added a bug in 4c7eb1b by using the clock fields without checking for their existence first. The solution in 37bb61c feels a bit hacky but I'm not sure of anything better.

Have you looked at how the other vendor packages solve this? E.g. Xilinx? I would expect that a similar code structure would work for Altera.

I have, but I found them very confusing and difficult to understand. You're welcome to refactor my code to fit in better though.

Sure, but you'll need to add DDR support first, even if it doesn't work.

Writing ba3dc3c felt like wading through mud; did Altera need to make the cells this difficult to use? I've not done tristate/bidirectional stuff yet because I'm not sure I can summon the motivation for it; it's just painful to work on.

I've done some more research over the altlvds cell, and I'm not convinced its semantics match up with nMigen's; the I/O cells for nMigen seem to be implicitly infallible without initialisation, but altlvds is fallible (PLLs may desynchronise causing data corruption) and requires initialisation (for PLL lock).

altlvds also has sparse availability. It should not be necessary to use a PLL to make a DDR receiver on any FPGA family, Altera or not...

Yeah it's not great, but I don't know of any better solutions.

Well I'm not actually convinced yet that altddio doesn't support differential receive pins. What does Quartus say about your combination of altddio and altiobuf?

It doesn't like it. Given:

module top(input clk, p_port, n_port, output i0, i1);

wire p_posedge, p_negedge, n_posedge, n_negedge;

altddio_in #(.width(1)) foo_ddr_p_0 (.datain(p_port), .inclock(clk), .dataout_h(p_posedge), .dataout_l(p_negedge));
altddio_in #(.width(1)) foo_ddr_n_0 (.datain(n_port), .inclock(clk), .dataout_h(n_posedge), .dataout_l(n_negedge));
altiobuf_in #(.NUMBER_OF_CHANNELS(1), .ENABLE_BUS_HOLD("FALSE"), .USE_DIFFERENTIAL_MODE("TRUE")) foo_buf_p_0 (.datain(p_posedge), .datain_b(n_posedge), .dataout(i0));
altiobuf_in #(.NUMBER_OF_CHANNELS(1), .ENABLE_BUS_HOLD("FALSE"), .USE_DIFFERENTIAL_MODE("TRUE")) foo_buf_n_0 (.datain(p_negedge), .datain_b(n_negedge), .dataout(i1));

endmodule

I get

Error (17044): Illegal connection on I/O input buffer primitive altiobuf_in:foo_buf_n_0|iobuf_in_l9g:auto_generated|ibuf
a[0]. Source I/O pin altddio_in:foo_ddr_p_0|ddio_in_d0b:auto_generated|ddio_ina[0] drives out to destinations other than
 the specified I/O input buffer primitive. Modify your design so the specified source I/O pin drives only the specified
I/O input buffer primitive. File: //wsl$/Ubuntu-18.04/home/lofty/de_10/build/db/iobuf_in_l9g.tdf Line: 33

OK. Looks like it is indeed not supported. So you should set xdrs=(0,1) for differential pins and xdrs=(0,1,2) for single-ended ones.

module top(clk50_0__io, p_port, n_port, i0, i1);
input clk50_0__io;
(* altera_attribute = "-name IO_STANDARD \"LVDS\" " *) input p_port, n_port;
output i0, i1;

wire buf_out;

altiobuf_in #(.NUMBER_OF_CHANNELS(1), .ENABLE_BUS_HOLD("FALSE"), .USE_DIFFERENTIAL_MODE("TRUE")) foo_buf_p_0 (.datain(p_port), .datain_b(n_port), .dataout(buf_out));
altddio_in #(.width(1)) foo_ddr_p_0 (.datain(buf_out), .inclock(clk50_0__io), .dataout_h(i0), .dataout_l(i1));

endmodule

This works okay. Guess the altddio documentation is wrong here.

Worth noting is that Quartus is extremely picky about the I/O standard here. Without the altera_attribute there, this code fails to compile.

That's normal. It should already be applied by nMigen elsewhere in platform code.

Presumably then I need to implement DDR tristate/bidirectional I/O for completeness?

Okay, I believe this is feature-complete. The resulting Verilog needs to be checked and tested though.

Looks good at a quick glance. Thank you for the contribution. I will check this on real hardware at some point soon, and then merge while adjusting anything that needs changing.