-
Notifications
You must be signed in to change notification settings - Fork 112
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
SRL16 support #961
SRL16 support #961
Conversation
096bcf4
to
19bec32
Compare
625da30
to
cf83d4a
Compare
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 awesome! Just some small comments.
|
||
if carry4 is None: | ||
return | ||
# Simplest check is if the CARRY4 has output in used by either the OUTMUX |
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.
Let's refactor the cleanup into "cleanup_carry4", "cleanup_srl", assuming that no logic/data is shared between them.
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.
Done.
<direct name="SLICEM_MODES.CE_to_WE_MUX.CE" input="SLICEM_MODES.CE" output="WE_MUX.CE"/> | ||
<direct name="SLICEM_MODES.WE_to_WE_MUX.WE" input="SLICEM_MODES.WE" output="WE_MUX.WE"/> |
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.
Why force CE
over WE
here?
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 is to maintain compatibility with Vivado which cannot route through WE
to an SRL.
There is no such limitation in hardware, both CE
and WE
work. However, when trying to import disassembled fasm with routing through WE
, Vivado refuses to use that input which results in "antenna net" error.
I added more elaborate comment to the file.
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 have misinterpreted the error. Vivado can route using the WE
to the SRL, but will generally refuse if the CE
port is available. The example to check is have the SRL CE and FF CE use different net's, but force them to pack into the same SLICEM
.
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.
Ok, I'll check that tomorrow. But still, if I enable WE
there will be issues with importing the VPR routing back to Vivado in case when there is only SRL in a slice.
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.
The solution to this problem is have VPR choose CE over WE, either by making sure it is the default (e.g. first in the <mux>
) or by some timing pressure. Presumably Vivado chooses CE over WE for that reason.
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've re-enabled the WE input of WEMUX.
Actually, the CE
input should be the default as it is defined in the XML:
<mux name="WE_MUX" input="WE_MUX.CE WE_MUX.WE" output="WE_MUX.WE_OUT">
<metadata>
<meta name="fasm_mux">
WE_MUX.CE = WEMUX.CE
WE_MUX.WE = NULL
</meta>
</metadata>
</mux>
@@ -0,0 +1,65 @@ | |||
<pb_type name="ASRL" num_pb="1" xmlns:xi="http://www.w3.org/2001/XInclude"> |
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.
Please add a comment on why the a_srl is special, for future reference.
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 special because it requires different pack patterns for chaining. I added comment at the beginning of the file.
…use VPR complained about them. Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
…ing. Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
…s for SRL16 (some). Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
…Yosys!). Reenabled testbinch for srl16_amc31 Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
…with Vivado which can't route through WE. Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
ddd13f8
to
47a7986
Compare
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
@@ -0,0 +1,46 @@ | |||
add_file_target(FILE basys3_doutmux_top.v SCANNER_TYPE verilog) | |||
#add_file_target(FILE basys3_dffmux_top.v SCANNER_TYPE verilog) |
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.
Why are the dffmux tests commented out?
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.
That is because I cannot verify it on hardware. The VPR refuses to use FF on the same slice as the SRL. I'll try to play more with pack patterns, maybe I can get it to do so.
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.
@litghost I discovered that in order to have such a pack pattern we would need to make different COMMON_SLICE and SLICE_FF pb_types for SLICEL and SLICEM. That is because MC31 related pack patterns cannot be specified inside SLICEL as this makes VPR report an error.
I made specialized COMMON_SLICE and SLICE_FF but still couldn't force VPR to pack SRL and FF in the same slice.
I think that having the DFFMUX.MC31 working is not a top priority. I can file an issue with a description what (supposedly) needs to be done.
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 that having the DFFMUX.MC31 working is not a top priority. I can file an issue with a description what (supposedly) needs to be done.
This makes sense to me for now. File the issue, add a FIXME near this comment, and then LGTM.
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 added comments to CMake files. There is the issue: #974
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.
Assuming this is working on hardware, LGTM
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
…es/third_party/symbiyosys-43a7996 build(deps): bump third_party/symbiyosys from `adacad7` to `43a7996`
Add support for SRL16 to VPR and fasm2bels with timings.