-
Notifications
You must be signed in to change notification settings - Fork 569
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
build/efinix: add a few IO primitives, IO constraints, sdc rework #2060
Conversation
…rework how the SDC are handled
Co-authored-by: Fin Maaß <[email protected]>
PLL's modifications look good to be able to use a ClockSignal instead of a string but I see one issue: When more than one module with a PLL are added to a gateware clk_name is not unique. Maybe it's required to append name with id (PLL id ?) |
Either that, and this also mean that the clock name get some extra "noise", |
I already did some try with an id. The problem is more when the script is executed to avoid conflict between signals. I'm agree this adds a bit of noise. @enjoy-digital has also propose to adds primitive type+id when a clock is created to see if it's a PLL, or clkin,... |
…s input_signal name when name is not provided and PLL is configured in CORE or INTERNAL mode, create_clkout: added PLL name in clk_name str
…clk_out_name. litex/build/efinix/ifacewriter.py: generate_lvds: when slow_clk/fast_clk are ClockSignal uses platform.clks to map between domain and signal name
Applied with few addtions. Thanks @Dolu1990 ! |
|
||
class EfinixDDRTristateImpl(Module): | ||
def __init__(self, platform, io, o1, o2, oe1, oe2, i1, i2, clk): | ||
assert oe1 == oe2 |
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.
this assert doesn't work right if oe1
and oe2
are a _Slice
, like in the litespi ddr phy
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.
If i remember well, one issue is that the efinix primitive only support 1 output enable signal (in ddr mode).
- Either when oe1 != oe2 it could fall back to DDR tristate emulation (et get very bad timings)
- Either it could be ignored and hope for the best
- Either it can keep the assert
Not sure what is the best.
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.
Another options would be to update litespi to avoid the usage of 2 oe signals
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 was already with changes like: litex-hub/litespi#60
The problem is this: oe1 = dq_oe[i], oe2 = dq_oe[i],
with dq_oe = Signal(len(pads.dq))
where oe1
and oe2
are only a `_Slice``od a Signal and not a whole Signal. the error comes from here: https://github.com/m-labs/migen/blob/832a7240ba32af9cbd4fdd519ddcb4f912534726/migen/fhdl/structure.py#L41
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.
Hoo then would just need to do a dq_oe[i] once before , and refer twice to its result ?
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.
this should fix it m-labs/migen#292
add a few IO primitives, IO constraints, but mainly it rework how the SDC are handled.
The main issue with how the efinix flow was handled, is that the constraints for the syncronous IO were missing. This break stuff for high frequency design in large FPGA (Ti375)
The efinity tool generate a template outflow/xxx.pt.sdc. With this patch, this xxx.pt.sdc is combined with the one generated from litex.
!!! WARNING !!!
There is one dirty thing is this PR.
This is the changes in efinix.py. when you setup a clock domain with a pll, this replace the clock domain "clk" with the new one created by the PLL.
This is done to ensure the constraints match with the efinix pll toplevel IO.
An alternative would be to provide a way to specify that a given litex clock domain should use the given name in the SDC file (instead of the clock domain "clk" name)
Let me know what you think about this, this can be reworked.
let's me know if the PR need to be splitted.