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

Added Arty S7 support #101

Merged
merged 2 commits into from Aug 17, 2020
Merged

Added Arty S7 support #101

merged 2 commits into from Aug 17, 2020

Conversation

Fatsie
Copy link
Contributor

@Fatsie Fatsie commented Aug 8, 2020

This is based on Arty A7 file. Some things are handled differently:

  • Define "cpu_reset" as default_rst.
  • Use openFPGALoader for programming the board.
  • Don't overload .bin generation; it does not seem to make a difference.
  • Generate also mcs file. This is used by openFPGALoader for programming
    into SPI Flash.

Arty S7-50 has been tested on the board by blinky test; Arty S7-25 only
bitstream generation, not on the board.

@Fatsie
Copy link
Contributor Author

@Fatsie Fatsie commented Aug 8, 2020

For openFPGALoader to work it needs this PR to work.

@Fatsie
Copy link
Contributor Author

@Fatsie Fatsie commented Aug 8, 2020

I played around a little and I think I can make the programming also work through a vivado script.
I think that may preferred over an external programming tool like openFPGALoader or xc3sprog ?

@Fatsie
Copy link
Contributor Author

@Fatsie Fatsie commented Aug 8, 2020

PR for generic vivado FPGA programming script: amaranth-lang/amaranth#468
With that change I can remove the toolchain_program() method from _ArtyS7Platform.

@whitequark
Copy link
Member

@whitequark whitequark commented Aug 9, 2020

I'd prefer if you put the toolchain_program function in the board file.

@Fatsie
Copy link
Contributor Author

@Fatsie Fatsie commented Aug 9, 2020

But I think it comes down to duplcating the same code for all boards using Vivado as toolchain. I would think centralizing it make it more maintainable.
Do you agree to use vivado and not xc3sprog or openFPGALoader for programming FPGA that use Vivado toolchain ?
Did now push change in extra commit, can squash it if you agree with the change.

@whitequark
Copy link
Member

@whitequark whitequark commented Aug 9, 2020

I would think centralizing it make it more maintainable.

I disagree that it improves maintainability. Because we can not automatically test changes to the common, factored-out code (as doing it faithfully requires hardware unique to that board, and the likelihood of anyone changing the factored-out code having all of this hardware approaches zero as the amount of boards grows), it is better to duplicate code, and have the peace of mind that any changes have in fact been tested by the person making the change.

Do you agree to use vivado and not xc3sprog or openFPGALoader for programming FPGA that use Vivado toolchain ?

We don't yet have a clear policy for this. It's clear to me that the default option would be whatever the board vendor suggests; so for Digilent Xilinx boards it should be Vivado. But it's also clear that a single option wouldn't suffice (especially seeing as many people are not fond of proprietary toolchains). So we probably should work out a way to support multiple programmers, but in the meantime, Vivado will do.

@DaKnig
Copy link
Contributor

@DaKnig DaKnig commented Aug 9, 2020

Related: I have changed my local board file to work with the Vivado programmer for the arty z7 board in a similar way to what you are suggesting, because I could not get xc3sprog to work (because of reasons I wont discuss here). While this approach did work, Vivado takes a few seconds to start before it can even start programming. this is very slow and I doubt xc3sprog requires that long to start up (because it doesn't have to start the tcl interpreter and what not). while having that option be in the board file might be a good idea, for those like me who cant use the free programmer, I am not sure that using vivado as the default is the best idea for programming the bitstream of the board even if diligent suggests that.

@Fatsie
Copy link
Contributor Author

@Fatsie Fatsie commented Aug 9, 2020

Related: I have changed my local board file to work with the Vivado programmer for the arty z7 board in a similar way to what you are suggesting, because I could not get xc3sprog to work (because of reasons I wont discuss here). While this approach did work, Vivado takes a few seconds to start before it can even start programming. this is very slow and I doubt xc3sprog requires that long to start up (because it doesn't have to start the tcl interpreter and what not). while having that option be in the board file might be a good idea, for those like me who cant use the free programmer, I am not sure that using vivado as the default is the best idea for programming the bitstream of the board even if diligent suggests that.

For me the use case for the build in nmigen board programming function is programming the board after you build the bitstream with nmigen. In that respect I see the extra two seconds that is added to the whole Vivado flow as negligible and people will have Vivado installed anyway.
For programming FPGAs with pre-build distributed bitstream files I would not advocate Vivado as tool to use. Current plan is to support openFPGALoader as it is included in open-tool-forge.

@Fatsie
Copy link
Contributor Author

@Fatsie Fatsie commented Aug 9, 2020

but in the meantime, Vivado will do.

OK, squashed the commits.

@whitequark
Copy link
Member

@whitequark whitequark commented Aug 9, 2020

Thanks. Everything looks good (aside from minor PEP8 violations, which I'll just fix when merging), except for one thing: I'm not quite sure what are you doing with cpu_reset here. Could you elaborate? Should Arty A7 be changed as well?

@Fatsie
Copy link
Contributor Author

@Fatsie Fatsie commented Aug 9, 2020

The Arty has two 'reset' signals, one connected to the program pin of the FPGA the other to an IO. Pressing the former will reload the bitstream from Flash and thus can be seen as hard reset. The latter is called cpu_reset. The name I took over from Arty A7 but I made it the default reset. This makes the blinky test react to the reset button.
Yes, I think it would make sense to align with Arty A7.

@whitequark
Copy link
Member

@whitequark whitequark commented Aug 9, 2020

Would you say that, based on your experience (presumably you use these Xilinx devboards--I don't, although I do own several), you'd expect cpu_reset to reset the HDL design, even though it doesn't necessarily have any CPUs? In that case yeah, please also submit a separate PR to fix Arty A7.

@Fatsie
Copy link
Contributor Author

@Fatsie Fatsie commented Aug 9, 2020

Yes, and now I see that in the Digilent schematic for Arty S7 the second reset is called 'ck_rst' which I assume stands for circuit reset. So maybe I should name the resource just rst ?

@whitequark
Copy link
Member

@whitequark whitequark commented Aug 9, 2020

So maybe I should name the resource just rst ?

Yes, according to our conventions it would be just rst.

This is based on Arty A7 file. Some things are handled differently:
* Rename cpu_reset resource to rst and use it as default circuit reset.
* Use Vivado for programming the board.
* Don't overload .bin generation; it does not seem to make a difference.
* Generate also mcs file. This is used by openFPGALoader for programming
  into SPI Flash.

Arty S7-50 has been tested on the board by blinky test; Arty S7-25 only
bitstream generation, not on the board.
@Fatsie
Copy link
Contributor Author

@Fatsie Fatsie commented Aug 9, 2020

Renamed cpu_reset -> rst; also corrected wrongly deleted toochain_prepare() in previous push.

@whitequark whitequark merged commit d20fb96 into amaranth-lang:master Aug 17, 2020
1 check failed
@whitequark
Copy link
Member

@whitequark whitequark commented Aug 17, 2020

Thanks!

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.

None yet

3 participants