-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Digilent Atlys spartan6 board #15
Conversation
(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 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.
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. |
OK, will adapt. I did prefer
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.
I also added |
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
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.
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.
You're right. That must be a mistake. I'll take another look on those boards.
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. |
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 |
I did find a (possible) solution for all original TODOs and pushed a new patch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's gtx_clk
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
nmigen_boards/atlys.py
Outdated
|
||
def toolchain_program(self, products, name): | ||
subprocess.run(["impact", "-batch", "{}.impact".format(name)], | ||
cwd=products._root, check=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I also needed to guarantee that the programming command was executed in the directory where the files are extracted.
nmigen_boards/atlys.py
Outdated
def toolchain_program(self, products, name): | ||
with products.extract("{}.bit".format(name)) as bitfile: | ||
cmd=""" | ||
setMode -bscan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: textwrap.dedent
allos to make this a bit less ugly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
nmigen_boards/atlys.py
Outdated
from textwrap import dedent | ||
|
||
from nmigen.build import * | ||
from nmigen.vendor.xilinx_spartan6 import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I am now using m-labs/nmigen#132 feature and also updated spartan module name. |
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? |
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.
I do think the jumper is configured to 2.5V when you buy the board. |
Testing this reveals that currently build can only be called once on a platform as you get the following exception on second call:
This actually makes me prefer to only support specifying jumper setting during construction. |
@whitequark |
Sorry, I was busy with other things, no changes needed. I'll get around to this PR soon. |
Thank you for your contribution, and I apologize for the delay. |
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.