Digilent Atlys spartan6 board #15

Fatsie · 2019-07-05T09:17:19Z

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 · 2019-07-05T13:49:01Z

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 · 2019-07-05T19:46:20Z

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 · 2019-07-05T19:58:09Z

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 · 2019-07-05T20:06:23Z

[...] 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 · 2019-07-06T12:56:49Z

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.

whitequark

Looks almost done. One thing I'd like to change is to add first-class support for voltage jumpers, using something like...

def bank2_iostandard(self):
    if self.JP12 == "2V5": return "LVCMOS25"
    else: return "LVCMOS33"
resources = [
    Resource("user_led", 7, ..., Attrs(IOSTANDARD=bank2_iostandard)
]

But your current implementation is also acceptable, so that's not a blocker.

whitequark · 2019-07-06T18:34:29Z

nmigen_boards/atlys.py

+            Subsignal("int",     PinsN("L16", dir="o")),
+            Subsignal("mdio",    Pins("N17", dir="io")),
+            Subsignal("mdc",     Pins("F16", dir="o")), # Max 8.3MHz
+            Subsignal("gtx_clk", Pins("L12", dir="o")),


What's gtx_clk?

It is a standard signal defined for GMII. In Gbit mode this is a 125MHz clock provided from MAC to PHY for tx. In 100/10Mbit the 25/2.5MHz clock tx_clk from PHY to MAC is used as for MII.

whitequark · 2019-07-06T18:37:53Z

nmigen_boards/atlys.py

+
+    def toolchain_program(self, products, name):
+        subprocess.run(["impact", "-batch", "{}.impact".format(name)],
+            cwd=products._root, check=True,


You should never access internal (starting with _) fields of BuildProducts (or anything else in nMigen) directly.

This is a problem here because while the current BuildProducts implementation provides all the files locally, I would like to add one in the future that performs the entire build remotely, such that products.get() performs a transfer over SSH. See the Versa ECP5 for an example of how to do it correctly.

Difficulty was that the generated file {{name}}.impact needs to refer to other generated file {{name}}.bit. I now switched to generate the commands inside toolchain_program()

I see, thanks. Another solution might be to guarantee that each file grabbed via extract ends up in the same directory. But it might not help enough.

And I also needed to guarantee that the programming command was executed in the directory where the files are extracted.

whitequark · 2019-07-06T21:08:19Z

nmigen_boards/atlys.py

+    def toolchain_program(self, products, name):
+        with products.extract("{}.bit".format(name)) as bitfile:
+            cmd="""
+setMode -bscan


Nit: textwrap.dedent allos to make this a bit less ugly.

whitequark

Very nice. I am approving this but not merging just yet. I'd like to add first-class support for dependent attributes soon. If I don't get around to do it in a day or two, please ping me and I'll merge as-is.

cr1901 · 2019-07-07T23:43:11Z

nmigen_boards/atlys.py

+from textwrap import dedent
+
+from nmigen.build import *
+from nmigen.vendor.xilinx_spartan6 import *


Needs to be changed to nmigen.vendor.xilinx_spartan_3_6 in light of Spartan 3A support being merged in nmigen.

This board file was developed based on the Atlys reference manual and the Atlys platform file from litex-buildenv.

Fatsie · 2019-07-08T20:52:54Z

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 · 2019-07-09T00:05:44Z

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 · 2019-07-09T19:51:46Z

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 · 2019-07-09T20:45:42Z

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 · 2019-07-16T19:08:13Z

@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 · 2019-07-16T19:09:22Z

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

whitequark · 2019-08-04T11:20:46Z

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

Fatsie marked this pull request as ready for review July 6, 2019 12:56

whitequark suggested changes Jul 6, 2019

View reviewed changes

whitequark reviewed Jul 6, 2019

View reviewed changes

whitequark approved these changes Jul 6, 2019

View reviewed changes

cr1901 reviewed Jul 7, 2019

View reviewed changes

Digilent Atlys Spartan6 board

a429799

This board file was developed based on the Atlys reference manual and the Atlys platform file from litex-buildenv.

Fatsie mentioned this pull request Jul 20, 2019

Submodule named 'ddr2_0__a' already exists after 698b005 with Atlys m-labs/nmigen#157

Closed

whitequark added 2 commits August 4, 2019 11:19

Update for upstream changes in nMigen.

a513b8d

Style. NFC

d1715be

whitequark merged commit 78f3594 into m-labs:master Aug 4, 2019

This was referenced Jan 27, 2020

Digilent Atlys spartan6 board amaranth-lang/amaranth-boards#19

Closed

Allow dependent platform attributes amaranth-lang/amaranth#134

Closed

Submodule named 'ddr2_0__a' already exists after 698b005 with Atlys amaranth-lang/amaranth#158

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Digilent Atlys spartan6 board #15

Digilent Atlys spartan6 board #15

Fatsie commented Jul 5, 2019

whitequark commented Jul 5, 2019

Fatsie commented Jul 5, 2019

whitequark commented Jul 5, 2019 •

edited

whitequark commented Jul 5, 2019

Fatsie commented Jul 6, 2019

whitequark left a comment

whitequark Jul 6, 2019

Fatsie Jul 6, 2019 •

edited

whitequark Jul 6, 2019

Fatsie Jul 6, 2019

whitequark Jul 6, 2019

Fatsie Jul 6, 2019

whitequark Jul 6, 2019

Fatsie Jul 6, 2019

whitequark left a comment

cr1901 Jul 7, 2019

Fatsie commented Jul 8, 2019

whitequark commented Jul 9, 2019

Fatsie commented Jul 9, 2019

Fatsie commented Jul 9, 2019 •

edited

Fatsie commented Jul 16, 2019

whitequark commented Jul 16, 2019

whitequark commented Aug 4, 2019

Digilent Atlys spartan6 board #15

Digilent Atlys spartan6 board #15

Conversation

Fatsie commented Jul 5, 2019

whitequark commented Jul 5, 2019

Fatsie commented Jul 5, 2019

whitequark commented Jul 5, 2019 • edited

whitequark commented Jul 5, 2019

Fatsie commented Jul 6, 2019

whitequark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fatsie Jul 6, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitequark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fatsie commented Jul 8, 2019

whitequark commented Jul 9, 2019

Fatsie commented Jul 9, 2019

Fatsie commented Jul 9, 2019 • edited

Fatsie commented Jul 16, 2019

whitequark commented Jul 16, 2019

whitequark commented Aug 4, 2019

whitequark commented Jul 5, 2019 •

edited

Fatsie Jul 6, 2019 •

edited

Fatsie commented Jul 9, 2019 •

edited