-
Notifications
You must be signed in to change notification settings - Fork 188
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
applet.program.ecp5_sram: Initial ecp5 SRAM applet #191
base: main
Are you sure you want to change the base?
Conversation
I see that you import bitarray, but don't see that it is used at all. Probably copied from somewhere else? bitarray has some problems so the plan is to deprecate it. So I would prefer if there were no new users of bitarray added to Glasgow. I didn't do a thorough review of the rest as I don't have enough experience with the ECP5 and JTAG. |
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.
Thanks for the PR! I hope this review shows what the applet is missing before it can be on the same footing as other similar applets upstream. If some of my references to *.xc9500xl
are too opaque I can elaborate on them, but I feel like just pointing to existing code might be easier.
Yep, bitarray is not needed. I was poking around with it to try to handle the bit reversal. I'll remove the import Thanks for the comments! I'll work through them now. Speed on the bit reversal isn't a huge impact. A bigger factor seems to be only calling
|
This PR has been sitting on my local branch for awhile now, would be useful to get it merged. Because it's been so long, I've rebased the changes, and squashed my working commits. |
Sorry for the delay, I had my hands full. I'll take a look soon! |
Anything I can do to help this along? |
@gregdavill Apologies for the huge delay--I'm currently working on updating this applet to the latest coding standards and working on merging it. I do have a few questions about the way you wrote it. |
|
||
# Send entire bitstream data into DR, | ||
# Bytes are expected MSB by the ECP5, so need to be reversed | ||
# Slit bitstream up into chunks just for improving JTAG throughput |
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.
Does this really improve JTAG throughput? Is it because the byte reversal operation is so slow it pays off to run it in parallel with actual JTAG upload?
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.
So the part that's slow isn't byte reversal but converting a gigantic bitstream back and forth from bytes to bits to bytes again (in the JTAG code). This is ... awkward.
|
||
|
||
# Check status | ||
# Note ECP5 will not release until STATUS is read |
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.
ECP5 will not release what? The DONE pin?
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 can double check this, I seem to recall that the device remained in a configuration state, as opposed to a running state.
@@ -0,0 +1,84 @@ | |||
from ...support.bits import * |
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.
Do you remember which document this data is taken from? (It needs to go into our documentation archive.)
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.
Namely, the LSC status register is described in FPGA-TN-02039, but I cannot find a document which lists IR values.
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.
Yes, in FPGA-TN-02039 ECP5 and ECP5-5G sysCONFIG Usage Guide
the commands listed under 6.5 Slave SPI Command Table. These values are used by the SPI access to sysCONFIG. They are the same as IR values used via JTAG.
The variable used to be called `args.bitstream`, which clashed with FPGA programming related applets; program-ice40-sram and the upcoming program-ecp5-sram. Rename it to `args.prebuilt_at` (as the option), reducing the likelihood of name clashes. Eventually we should have proper namespacing here, but that's a lot more work than I can do now.
An issue I found while testing this applet is that it does not (as written) assert PROGRAMN at the beginning of configuration, so if e.g. there was already a bitstream, or if a bitstream load finished with an error, this state is preserved even if another bitstream load is performed. |
@gregdavill I've updated the applet to follow all of our standards and guidelines; it is almost ready to be merged! There are only a few items left. Functionality:
Documentation:
|
@gregdavill Friendly ping--I'd like to merge this! I've been explicitly allocating resources for Glasgow maintenance recently so things should go faster. |
Thanks for the ping. I had totally not seen the work you put into this in October. Thanks.
I can take a look into this. Not that the sysConfig block exposes the same (or a subset?) register set via both SPI and JTAG. I have found it useful to use to the "SPI" commands/timing and registers even when using JTAG. The docs don't appear to make this obvious. The programming flow listed in 6.10 appears to claim that toggling PROGRAMN and clocking in the REFRESH command are equivalent, I've not checked this claim.
I've taken the SPI commands, as listed FPGA-TN-02039 section 6.5
IIRC Initially nextpnr-ecp5 ecppack, as it has an option to export a programming SVF,. However the flow is essentially the same as described in FPGA-TN-02039 section 6.10.
I think 10 cycles was a guess that worked, or it was used by the SVF. |
@gregdavill Hi! Pinging you again on these changes. For documentation I'd like it to be added to our archive and the header to be updated to list it, the same way it's done in other files. For the functional difference of 24/10 clocks I'd like that to be added to the applet. |
I've started working on an applet for the ecp5.
Right now it supports the bare minimum: reading IDCODE, STATUS register and can download a bitstream.
The ECP5 expects the bit stream data to be sent MSB first, which is how the SPI flash is operating, but reverse to JTAG. So we need to reverse every byte before we send it out, I have a very crude implementation of this right now.