Skip to content
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

vendor.lattice_machxo_2_3l: fix sdc generation #547

Closed
wants to merge 2 commits into from

Conversation

anuejn
Copy link
Contributor

@anuejn anuejn commented Nov 19, 2020

This PR fixes both issues reported in #546 .
Probably there is a similiar issue for ecp5 and ice40 since they both use synplify.

@anuejn anuejn changed the title vendor.lattice_machxo_2_3l: fix sdl generation vendor.lattice_machxo_2_3l: fix sdc generation Nov 19, 2020
@codecov
Copy link

codecov bot commented Nov 19, 2020

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (05ac367) 87.15% compared to head (50de645) 87.11%.

Files Patch % Lines
amaranth/build/plat.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #547      +/-   ##
==========================================
- Coverage   87.15%   87.11%   -0.04%     
==========================================
  Files          41       41              
  Lines        7427     7430       +3     
  Branches     1767     1767              
==========================================
  Hits         6473     6473              
- Misses        778      780       +2     
- Partials      176      177       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@whitequark
Copy link
Member

Why not use tcl_quote?

@anuejn
Copy link
Contributor Author

anuejn commented Nov 20, 2020

I thought that this way it is more clear that we are working around a bug here.
But I can change that.

@anuejn
Copy link
Contributor Author

anuejn commented Nov 20, 2020

Also if we simply chain tcl_escape and tcl_quote, we end up transforming \ to \\\\

@whitequark
Copy link
Member

That function is also for working around a similar issue in Vivado...

@anuejn
Copy link
Contributor Author

anuejn commented Nov 20, 2020

So is it safe to just use tcl_quote even if it doesnt escape { and }?

@whitequark
Copy link
Member

Yes, tcl_escape and tcl_quote are mutually exclusive and they have the same function.

@whitequark
Copy link
Member

whitequark commented Nov 20, 2020

Please do the same for (at least) ECP5, though if you have iCECube, then iCE40 as well, and verify that it works.

@anuejn
Copy link
Contributor Author

anuejn commented Nov 20, 2020

Should I do it as part of this pr?

@whitequark
Copy link
Member

Yup.

@anuejn
Copy link
Contributor Author

anuejn commented Nov 22, 2020

What we need here is actually really something different than tcl_quote. It seems like synplify really expects the net to be wrapped in curly braces but escaped as if they were wrapped in quotation marks. Just using tcl_quote doesnt work :(.

@whitequark
Copy link
Member

whitequark commented Nov 23, 2020

Ok well that's horrible (and explains why I couldn't get it right several times already...) In that case please add sdc_quote or something like that. (I strongly suspect this requirement is exclusive to sdc files, though it's been a while since I experimented with this stuff.)

@whitequark
Copy link
Member

@anuejn Have you had a chance to give this another look?

@anuejn
Copy link
Contributor Author

anuejn commented Dec 31, 2020

sorry for the long delay. the code I pushed now seems to work for me.

Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I ask you to split this PR into two separate commits? As far as I understand/remember (correct me if I'm wrong), the SDC quoting issue and the hierarchy separator issue are different, so it would be a lot easier to understand the VCS history later if they were split. In cases like this it's OK to do cross-cutting changes through the entire vendor/ subdirectory; you don't need to change any platforms you aren't familiar with.

@@ -395,6 +395,9 @@ def tcl_escape(string):
def tcl_quote(string):
return '"' + re.sub(r"([$[\\])", r"\\\1", string) + '"'

def sdc_quote(string):
return "{" + re.sub(r"([${}\\])", r"\\\1", string) + "}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have a comment here explaining that this separate method is necessary because SDC does not actually behave like Tcl.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't the whole point of adding this function the fact that all Synplify frontends use this quoting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would be ice40 in addition to the two I already changed, right?
sadly I do not have icecube installed so i cannot test easily. should I change it anyways?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a testcase handy? I can check all of the currently supported toolchains since I have all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops I had this comment sitting here since the beginnig of 2021 but didnt send it *self facepalm*. Here you go:

I am using the following code to test it with diamond. Then I check in build/top_impl/top_impl_scck.rpt if the clock was constrained to 2Mhz successfully.

from nmigen import *
from nmigen.build import *
from nmigen_boards.tinyfpga_ax2 import *
from nmigen_boards.orangecrab_r0_2 import *
import sys

class Test(Elaboratable):
    def elaborate(self, plat):
        m = Module()

        m.submodules.jtag = JTAG()

        return m

class JTAG(Elaboratable):
    def elaborate(self, plat):
        m = Module()

        domain_name = "$funky_domain_name"
        cd_sync = ClockDomain(domain_name)
        m.domains += cd_sync
        plat.add_clock_constraint(cd_sync.clk, 2e6)

        if isinstance(platform, TinyFPGAAX2Platform):
            plat.add_resources([Resource("gpio", 0, Pins("1", conn=("gpio", 0), dir="o"), Attrs(IOSTANDARD="LVCMOS33"))])
            led = plat.request("gpio", 0)
        else:
            plat.add_resources([Resource("gpio", 0, Pins("1", conn=("io", 0), dir="o"), Attrs(IOSTANDARD="LVCMOS33"))])
            led = plat.request("gpio", 0)
        m.d[domain_name] += led.eq(~led)

        m.submodules.inst = Instance("JTAGF" if isinstance(platform, TinyFPGAAX2Platform) else "JTAGG", o_JTCK=cd_sync.clk)

        return m

for platform in [OrangeCrabR0_2Platform(toolchain="Diamond"), TinyFPGAAX2Platform()]:
    print(platform, file=sys.stderr)
    platform.build(Test())

synplify used in lattice diamond requires different quoting than standard tcl
quoting: wrapped in curly braces but escaped as if wrapped in quotation marks.
@whitequark
Copy link
Member

I don't believe this PR fixes the issue it claims to fix. I still get this error:

@W: MT548 :"/home/whitequark/Projects/amaranth/build/top.sdc":2:0:2:0|Source for clock clk not found in netlist.

@whitequark
Copy link
Member

I've cherry-picked 306eb7e into #1177 (iCE40 needs the same fix, I think).

I'm going to close this PR in favor of that one since the sdc_quote function introduced here doesn't fix the bug and it also doesn't make any sense to me that it would, given Tcl quoting rules. Note that #1177 also changes quoting in a different way (to only use "" strings) which may or may not be a part of the solution.

@whitequark whitequark closed this Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants