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

applet.program.ecp5_sram: Initial ecp5 SRAM applet #191

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gregdavill
Copy link
Contributor

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.

@electroniceel
Copy link
Member

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.

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.

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.

software/glasgow/applet/program/ecp5_sram/__init__.py Outdated Show resolved Hide resolved
software/glasgow/applet/program/ecp5_sram/__init__.py Outdated Show resolved Hide resolved
software/glasgow/applet/program/ecp5_sram/__init__.py Outdated Show resolved Hide resolved
software/glasgow/applet/program/ecp5_sram/__init__.py Outdated Show resolved Hide resolved
software/glasgow/applet/program/ecp5_sram/__init__.py Outdated Show resolved Hide resolved
software/glasgow/applet/program/ecp5_sram/__init__.py Outdated Show resolved Hide resolved
software/glasgow/applet/program/ecp5_sram/__init__.py Outdated Show resolved Hide resolved
software/glasgow/applet/program/ecp5_sram/__init__.py Outdated Show resolved Hide resolved
software/glasgow/applet/program/ecp5_sram/__init__.py Outdated Show resolved Hide resolved
software/glasgow/applet/program/ecp5_sram/__init__.py Outdated Show resolved Hide resolved
@gregdavill
Copy link
Contributor Author

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 shift_tdi with 8 bits at a time.

Here is a quick benchmark (552K bitstream, 6MHz TCK) for 2 bit-reversal methods.

Control ( _reverse() ): 14.2s
_lookup_table():        13.7s

Calling shift_tdi with 64bytes. 512 bits
_reverse():      4.7s
_lookup_table(): 4.0s

@gregdavill
Copy link
Contributor Author

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.

@whitequark
Copy link
Member

Sorry for the delay, I had my hands full. I'll take a look soon!

Base automatically changed from master to main February 27, 2021 09:59
@gregdavill gregdavill changed the title RFC: applet.program.ecp5_sram: initial ecp5 applet applet.program.ecp5_sram: Initial ecp5 SRAM applet May 1, 2021
@gregdavill
Copy link
Contributor Author

Anything I can do to help this along?

@whitequark
Copy link
Member

@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
Copy link
Member

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?

Copy link
Member

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
Copy link
Member

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?

Copy link
Contributor Author

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 *
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 remember which document this data is taken from? (It needs to go into our documentation archive.)

Copy link
Member

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.

Copy link
Contributor Author

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.
@whitequark
Copy link
Member

whitequark commented Oct 28, 2023

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.

@whitequark
Copy link
Member

@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:

  • The applet does not assert PROGRAMN at the beginning of configuration. This is probably done through some register, but without documentation (per items below) it's hard to figure out which.

Documentation:

  • Where did you obtain the encodings for IR_* instructions in glasgow.arch.lattice.ecp5?
  • Where did you obtain the programming algorithm for bitstream burst load?
  • Are the 10 cycles in Run-Test/Idle specified by some document (which?) or is this just a guess?

@whitequark
Copy link
Member

@gregdavill Friendly ping--I'd like to merge this! I've been explicitly allocating resources for Glasgow maintenance recently so things should go faster.

@gregdavill
Copy link
Contributor Author

Thanks for the ping. I had totally not seen the work you put into this in October. Thanks.
I've not taken a look at this code for a long while, thought I'd reply with answers now.
I'll get my glasgow set up again and try out the changes soon.

Functionality:

  • The applet does not assert PROGRAMN at the beginning of configuration. This is probably done through some register, but without documentation (per items below) it's hard to figure out which.

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.

Documentation:

  • Where did you obtain the encodings for IR_* instructions in glasgow.arch.lattice.ecp5?

I've taken the SPI commands, as listed FPGA-TN-02039 section 6.5

  • Where did you obtain the programming algorithm for bitstream burst load?

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.

  • Are the 10 cycles in Run-Test/Idle specified by some document (which?) or is this just a guess?

I think 10 cycles was a guess that worked, or it was used by the SVF.
But in FPGA-TN-02039 section 6.6: LSC_ENABLE/LSC_DISABLE are classed as type C commands, these list 24 dummy clocks after the opcode, but also state that "command action starts on 3rd dummy clock.".

@whitequark
Copy link
Member

@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.

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

Successfully merging this pull request may close these issues.

3 participants