Digilent Atlys spartan6 board

[Fatsie]

This is current state for the Digilent Atlys board file. It is not finished but put forward to have feedback to solve some questions. The questions are put as TODO comments in the python code.

I used #4 (Arty A7) as inspiration but changed some things for the ethernet. Some changes: tx_col->col, tx_crs->crs. I'm wondering if we should also call tx_data and rx_data as txd and rxd according to standard. Other change is that I defined most of the signals defined as clock also with Clock() except for mdc signal. I noticed that it is needed when one actually wants to use one of these input signals as a clock.

[whitequark]

Some changes: tx_col->col, tx_crs->crs. I'm wondering if we should also call tx_data and rx_data as txd and rxd according to standard.

(Did you mean rx_col and rx_crs?)

The current naming (the one used in Versa and Arty) I did for *MII is deliberate. The reason I renamed col to rx_col is to indicate that it is also driven by the PHY, like other status bits such as rx_dv. The reason I renamed txd to tx_data is just for consistency with the rest.

You could argue that this renaming should not be done and all names should be consistent with the standard. But I think that's not a very good argument. Elsewhere in nMigen I chose to provide a consistent abstraction or wrapper over inconsistent vendor primitives as well; and moreover, the standards themselves are usually not very consistent either, differing between revisions, generations, or vendors. Besides, when writing code, one has to look up the exact pin names anyway, because e.g. cs# is not a valid Python field name.

Other change is that I defined most of the signals defined as clock also with Clock() except for mdc signal. I noticed that it is needed when one actually wants to use one of these input signals as a clock.

Because the clocks can change between 1 Gbps, 100 Mbps and 10 Mbps, I did not add hardcoded constraints in the board file elsewhere. (Maybe you can't get your gateware to work at 1 Gbps PHY rate, but you also don't care about it.)

Note that you can always do (and is, in fact, supposed to do) something like:

eth_rmii = platform.request("eth_rmii")
platform.add_clock_constraint(eth_rmii.tx_clk, 125e6)

Moreover, output signals should not be defined as clocks because that has no purpose. (A clock generated on the FPGA already has parameters that are well known to the synthesis and PAR tools, provided that every clock that is an input to the FPGA is also known.) So you shouldn't define those.

[Fatsie]

The current naming (the one used in Versa and Arty) I did for *MII is deliberate. The reason I renamed col to rx_col is to indicate that it is also driven by the PHY, like other status bits such as rx_dv. The reason I renamed txd to tx_data is just for consistency with the rest.

OK, will adapt. I did prefer col naming as for me collision is related to both rx and tx.

Other change is that I defined most of the signals defined as clock also with Clock() except for mdc signal. I noticed that it is needed when one actually wants to use one of these input signals as a clock.

Because the clocks can change between 1 Gbps, 100 Mbps and 10 Mbps, I did not add hardcoded constraints in the board file elsewhere. (Maybe you can't get your gateware to work at 1 Gbps PHY rate, but you also don't care about it.)

Note that you can always do (and is, in fact, supposed to do) something like:

eth_rmii = platform.request("eth_rmii")
platform.add_clock_constraint(eth_rmii.tx_clk, 125e6)

Will adapt. I am curious though how difficult it will be to write a generic ethernet IP block that does not need to know device specific things itself.

Moreover, output signals should not be defined as clocks because that has no purpose. (A clock generated on the FPGA already has parameters that are well known to the synthesis and PAR tools, provided that every clock that is an input to the FPGA is also known.) So you shouldn't define those.

I also added Clock() on output as I saw this done for the eth_clk25 and eth_clk50 resource for Arty A7 in #4 and also for the ethernet clocks for the Versa ECP5. For Versa ECP5 I am a little confused as I see there quite some Pins() that don't specify a direction. Is this not needed ?

[whitequark]

I did prefer col naming as for me collision is related to both rx and tx.

Actually, I think you're right in more than one aspect here. First, it is because the collision signal matters primarily to the transmitter (I'm not sure why I called it rx_col in the first place; the receiver should just reject it by CRC, in most cases, I think). Second, and even more importantly, there is the aspect of MII that I managed to completely miss, and it's this:

The CRS and COL signals are asynchronous to the receive clock, and are only meaningful in half-duplex mode. Carrier sense is high when transmitting, receiving, or the medium is otherwise sensed as being in use. If a collision is detected, COL also goes high while the collision persists.

So these signals aren't synchronized to either tx or rx clock, and they definitely should stay unprefixed for that reason alone. My earlier code is wrong and needs to be fixed too.

I am curious though how difficult it will be to write a generic ethernet IP block that does not need to know device specific things itself.

The idea with the nMigen platform redesign is that any Ethernet MAC with any PHY (other than high speed serial like SGMII or XAUI... an entirely different can of worms) should not know any device specific things.

I also added Clock() on output as I saw this done for the eth_clk25 and eth_clk50 resource for Arty A7 in #4 and also for the ethernet clocks for the Versa ECP5.

You're right. That must be a mistake. I'll take another look on those boards.

For Versa ECP5 I am a little confused as I see there quite some Pins() that don't specify a direction. Is this not needed ?

It's optional (because e.g. GPIOs do not have a specific intrinsic direction) but the missing directions on Versa ECP5 are definitely my mistake, and it came up earlier. I've made #16 to track that.

[whitequark]

[...] the ethernet clocks for the Versa ECP5.

Note that the clk125 pin of the PHY on Versa ECP5 is an output of the PHY, and so it is an input of the FPGA, which I just checked. So it is appropriate to have a Clock() attribute on it.

[Fatsie]

I did find a (possible) solution for all original TODOs and pushed a new patch. I consider it now ready for review and open for code changes requests.

[Fatsie]

I am now using m-labs/nmigen#132 feature and also updated spartan module name. Also added in commit message reference to litex-buildenv as that was also used as guidance for this board file.

[whitequark]

OK, this is pretty much perfect. I only have one remaining concern: should we put the logic for jumpers into the constructor, or maybe into a property instead? I'm not sure exactly.

Does this board have a default jumper setting?

[Fatsie]

I only have one remaining concern: should we put the logic for jumpers into the constructor, or maybe into a property instead?

I like to be able give it for to the constructor but I do think it would be nice to also be able to set the value between different builds; e.g. have a script that generates both a bitstream for the 2.5V and the 3.3V setting. I'll do a proposal.

Does this board have a default jumper setting?

I do think the jumper is configured to 2.5V when you buy the board.

[Fatsie]

I do think it would be nice to also be able to set the value between different builds; e.g. have a script that generates both a bitstream for the 2.5V and the 3.3V setting. I'll do a proposal.

Testing this reveals that currently build can only be called once on a platform as you get the following exception on second call:

Traceback (most recent call last):
  File "./jtag_test.py", line 104, in <module>
    products_2v5 = p.build(test, do_program=True, build_dir="build_2v5")
  File "/home/verhaegs/anaconda2/envs/migen_dev/lib/python3.6/site-packages/nmigen/build/plat.py", line 47, in build
    plan = self.prepare(fragment, name, **kwargs)
  File "/home/verhaegs/anaconda2/envs/migen_dev/lib/python3.6/site-packages/nmigen/build/plat.py", line 58, in prepare
    assert not self._prepared
AssertionError

This actually makes me prefer to only support specifying jumper setting during construction.

[Fatsie]

@whitequark Are you still waiting on changes from me? As said in last comment I don't see much use in making JP12 a property if a platform can only be used once to build a design.

[whitequark]

Sorry, I was busy with other things, no changes needed. I'll get around to this PR soon.

[whitequark]

Thank you for your contribution, and I apologize for the delay.