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

Spartan7 #1155

Closed
wants to merge 6 commits into from
Closed

Spartan7 #1155

wants to merge 6 commits into from

Conversation

bunnie
Copy link

@bunnie bunnie commented Nov 25, 2019

This is a PR just to help understand the things I did to make Spartan7 run -- mostly for you to see a quick diff of the changes and give me guidance on how to format this better. Don't integrate it as-is, it'll break your other builds.

If you want a PR on the database I could do that too but the fuzzing run stopped because it ran out of disk space. Any idea roughly how many gigabytes I should allocate for a full fuzzing run? It's eaten through 20+gigs so far and all this runs in a VM, and I prefer to allocate just enough space for the job at hand to make backups easier.

…aram

When compiling the cfg fuzzer sub-test, it looks like perhaps the
spartan7 requires that the user confg parameter and the location
of the BSCANE2 block be correlated: that is,

BSCANE2 at X0Y0 => user register 1
BSCANE2 at X0Y1 => user register 2
...

and so forth. Assigning register 2 to the block at X0Y0 makes
vivado throw an error

This patch adds a workaround to incremente the block and user
register together.
@@ -34,6 +34,8 @@ def run():
jtag_chains = ("1", "2", "3", "4")
for (tile_name, site_name), isone in zip(sites,
util.gen_fuzz_states(len(sites))):
site_name = site_name[:-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is confusing, can you write a comment explaining why you are doing this?

Copy link
Author

Choose a reason for hiding this comment

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

It looks like perhaps the spartan7 requires that the user config parameter and the location of the BSCANE2 block be correlated: that is,

BSCANE2 at X0Y0 => user register 1
BSCANE2 at X0Y1 => user register 2
...

This code here takes the name (BSCANE2_X0Y0), chops off the last digit (which is always 0 in the original code), and adds a sequence number that increments with the USER register parameter, thus creating a correlation of the block location and the user register number.

Basically, the original code as written would take the same block (BSCANE2 at X0Y0) and assign once to each of the user registers 1-4, this code correlates the four possible positions of the BSCANE2 to one each of the user registers.

Copy link
Author

Choose a reason for hiding this comment

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

It's basically a really dumb way to derive the name of the block. Probably a better way would be to figure out how to extract the name from the tile database without having to edit it using string operations, but I couldn't figure that out.

Copy link

Choose a reason for hiding this comment

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

This same issue showed up when I ran the existing artix7 fuzzers with a newer version of Vivado. I suspect it may be a result of the Vivado version used, rather than being a spartan vs artix difference.

propagate_IOB_SING(database, tiles_by_grid)
propagate_IOI_SING(database, tiles_by_grid)
propagate_IOI_Y9(database, tiles_by_grid)
#propagate_IOB_SING(database, tiles_by_grid)
Copy link
Contributor

Choose a reason for hiding this comment

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

Put this behind a os.getenv('XRAY_PART') != 'spartan7'

Copy link
Author

Choose a reason for hiding this comment

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

Yah, I can do that -- however, that's just a work-around. There are in fact the "SING" blocks on the top and bottom of each column, but it looks like the mapping is just slightly different from what is done on the Artix. I couldn't understand the code inside propagate_IOB_SING well enough though to figure it out, so I just commented it out to get the fuzzers to start running.

Given that, is there a more clever way to flag this as a "todo" item, rather than just commenting it out (which might lead us to forget that this needs to be fixed down the road)?

@tmichalak
Copy link
Contributor

Hi @bunnie, were you able to make any progress on the Spartan7 front? Do you need any additional hints?

@tmichalak
Copy link
Contributor

Superseded by #1535

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants