-
Notifications
You must be signed in to change notification settings - Fork 114
Add SERDES pairs and connectors #55
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
Conversation
@@ -15,20 +15,117 @@ class ECP55GEVNPlatform(LatticeECP5Platform): | |||
speed = "8" | |||
default_clk = "clk12" | |||
default_rst = "rst" | |||
def __init__(self, *, VCCIO1="LVCMOS25", VCCIO2="LVCMOS33", VCCIO6="LVCMOS33", VCCIO7="LVCMOS33", **kwargs): |
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.
These names are a bit confusing--"LVCMOS25" is not a voltage on VCCIO1 but rather an IO standard. I think the constructor should take voltages as arguments--the Atlys board uses strings like "2V5"
, so it'd be nice to be consistent here. Alternatively the constructor could take resistor positions instead, but it seems like voltage would be less confusing here.
self._VCCIO6 = VCCIO6 | ||
self._VCCIO7 = VCCIO7 | ||
|
||
def VCCIO1(self): |
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.
These functions really return an IO standard, not a voltage, so they should be called something like bank1_iostandard
like in the Atlys board.
|
||
|
||
# The switches and LEDs are connected over resistors to VCCIO6 for SW4 and SW5 switch 1-3 and VCCIO1 for the LEDs and SW5 switch 4-8. | ||
# VCCIO1 is connected by default to 2.5 V over R100 (can be set to 3.3 V by disconnecting R100 and connecting R105) |
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 think these two comments about VCCIO would make more sense as a constructor docstring.
Resource("serdesclk", 1, DiffPairs("Y19", "W20", dir="o")), # 200 MHz | ||
|
||
# Diff pairs, they have selectable termination resistors | ||
# Resource("diff", "0", DiffPairs(A4, A5), Attrs(IO_TYPE=VCCIO7)), |
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 apologize for making you do extra work because I misremembered how this board works. While the SERDES pins should indeed be described as resources (since every application will have to add the same resources anyway), the diffpairs should not be (since every application will use an application-specific resource incorporating these).
Subsignal("rx", DiffPairs("Y16", "Y17", dir="i")), | ||
), | ||
|
||
Resource("serdesclk", 0, DiffPairs("Y11", "Y12", 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.
According to the naming conventions we use, these should rather be called serdes_clk
.
# TODO: add other resources | ||
] | ||
connectors = [ | ||
# TODO: add connectors | ||
Connector("J", 39, "- - - D15 B15 C15 B13 B20 D11 E11 B12 C12 D12 E12 C13 D13 E13 A14 A9 B10 - - - - - - - - E7 - A11 - A19 - - - - - - -"), |
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.
In the previous PR I suggested you leave out the connector changes so that the extra work of cross-checking these during review (which could take a while) would not block merging the rest of the PR. But it's fine to leave it this way too.
Superseded by #56. |
Change the comments as you see fit, I think the differential pairs make more sense on a connector, but I've added them as commented out Resources just in case.