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

Jump to bottom

Arty S7 OpenOCD Support #111

Merged
merged 3 commits into from Oct 15, 2020
Merged

Arty S7 OpenOCD Support #111

merged 3 commits into from Oct 15, 2020

Conversation

cr1901
Copy link
Contributor

@cr1901 cr1901 commented Sep 13, 2020

This PR does two things:

  1. Fixes a typo that prevented the import from nmigen_boards.arty_s7 import * from working properly.
  2. Adds support for the openocd programmer for the Arty S7 board.

I kept in mind your comments re: OpenOCD and programmers in general when writing this PR. So perhaps consider it a litmus test to supporting more than one programmer in nmigen :):

I looked at the openocd invocations currently in nmigen-boards. All of them are different, pretty significantly so.

Indeed, all the openocd invocations are different, and this one's no exception. One difference is that I use the upstream board config file since it's available already (I committed it in mid 2018). Another difference is that I support both JTAG programming and flashing via a proxy.

We can just decide on what to do here. For example, we can decide that on every board with multiple ways to program it, toolchain_program takes a programmer="" keyword argument.

I went ahead and used this suggestion as-is.

toolchain_program should be configurable enough, and just enough, to cover the most typical use cases.

In addition, I added a flash keyword argument that lets the user choose between JTAG programming and flashing a bitstream. JTAG programming is noticeably faster, shortening the already long "write, compile, program" feedback loop. Hopefully at some point Vivado's time can be shaved off too :). Therefore I think this is a reasonable keyword argument to add that will cover most use cases. I don't know if vivado's programmer has an equivalent.

whitequark
whitequark requested changes Sep 13, 2020
View reviewed changes
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.

Unfortunately this ignores the flash argument if Vivado is chosen as the programmer. At the very least it must assert, but ideally it should actually implement that.

@cr1901
Copy link
Contributor Author

cr1901 commented Sep 13, 2020

cc: @Fatsie Do you have any experience with programming flash via Vivado? It looks like the program_hw_cfgmem command (page 977) can do the trick, but seeing as Vivado is remote for me, I can't really test this.

@Fatsie
Copy link
Contributor

Fatsie commented Sep 14, 2020

@cr1901 I have minimal experience with Vivado; for the script I just looked up some commands and fiddled until the script did what it should do. You should be able to do the same unless you don't have Vivado installed.

@Fatsie
Copy link
Contributor

Fatsie commented Sep 14, 2020

Also apologize for being terse; I am busy with total different things ATM and I don't expect to look at this deeper in the near future.

@cr1901
Copy link
Contributor Author

cr1901 commented Sep 14, 2020 •
edited

You should be able to do the same unless you don't have Vivado installed.

I already said I don't have Vivado installed.

Anyways, @whitequark, can I please assert on this for now? I got someone to help test remotely while I coded a TCL script, but I just cannot get flash programming to work with Vivado. This is the closest I came up with:

open_hw_manager
connect_hw_server
open_hw_target
current_hw_device [lindex [get_hw_devices] 0]
set cfgMem [create_hw_cfgmem -hw_device [current_hw_device] s25fl128sxxxxxx0-spi-x1_x2_x4]
set_property PROGRAM.FILES top-norgb.bin $cfgMem
set_property PROGRAM.ADDRESS_RANGE {use_file} $cfgMem
set_property PROGRAM.BLANK_CHECK 1 $cfgMem
set_property PROGRAM.ERASE 1 $cfgMem
set_property PROGRAM.CFG_PROGRAM 1 $cfgMem
set_property PROGRAM.VERIFY 1 $cfgMem
program_hw_cfgmem $cfgMem

which bombs with:

ERROR: [Labtools 27-3347] Flash Programming Unsuccessful: Failure to set flash parameters.
ERROR: [Common 17-39] 'program_hw_cfgmem' failed due to earlier errors.

I have no idea what Vivado is complaining about; this is the correct flash, according to the Digilent manual. My ability to test is limited; I want someone else to add this functionality.

@whitequark
Copy link
Member

Anyways, @whitequark, can I please assert on this for now?

Like I said, that's the bare minimum, so you can do that.

whitequark
whitequark reviewed Sep 17, 2020
View reviewed changes
nmigen_boards/arty_s7.py Outdated
@@ -164,6 +164,7 @@ def toolchain_program(self, product, name, *, programmer="vivado", flash=True):
assert programmer in ("vivado", "openocd")

if programmer == "vivado":
assert not flash
Copy link
Member

Choose a reason for hiding this comment

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

Something like this would be nicer:

Suggested change
assert not flash
assert not flash, "Flash programming is not supported with Vivado yet"

@cr1901
Copy link
Contributor Author

cr1901 commented Sep 17, 2020 •
edited

@whitequark I had another day where someone could help remotely; this time, I got Flash programming working with the caveat that the user needs to manually reset/power-cycle the board. This appears to be a limitation of the Vivado TCL API, as noted in a comment.

whitequark
whitequark approved these changes Sep 21, 2020
View reviewed changes
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.

Great work, and thanks for getting Vivado flashing done; this will definitely be useful for other boards in the future.

@whitequark whitequark merged commit bcc1467 into amaranth-lang:master Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whitequark whitequark whitequark approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cr1901 @Fatsie @whitequark