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

Option to factor out "clk" from "spi_flash" on Lattice ECP5 #38

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Dec 23, 2019

According to Section 6.1.2 of an official ECP5 manual, only the on-chip oscillator MCLK is available as the SPI clock when in Master SPI mode by default. To use any user clock as the SPI clock, a USRMCLK Instance must be instantiated and there is no need to request for the clock from Ball U3. Requesting for such clock will lead to an error as reported by @xobs on the nextpnr repo.

Therefore, I would like to suggest modification on memory.py such that there is an option to add the SPI flash resources to a platform (not only ECP5) without the clock. Although this modification has been used to successfully build and flash a bitstream on an ECP5 board (the example was an SPI reader module on my nmigen-stdio fork), I cannot deny that there is room for improvement on my code for this pull request.

I look forward to seeing further comments, thanks.

See also: my pull request on nmigen-stdio for a SPI controller.

@sbourdeauducq
Copy link
Member

Shouldn't nmigen-boards (or nmigen) instantiate USRMCLK to deal with this case transparently for the user?

@sbourdeauducq
Copy link
Member

Note that sometimes such primitives combine many unrelated signals (and you can always count on Xilinx for doing that, see STARTUPE3), so this needs some thought. Maybe a central place where all those primitives are managed and where the user can access them (e.g. connect additional signals, modify their parameters) if required.

)


class VersaECP5Platform_USRMCLK(_VersaECP5PlatformBase):
Copy link
Member

Choose a reason for hiding this comment

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

Probably better to add a parameter to __init__ instead of having two separate classes. This is what we did in other places (e.g. Migen), having one class for each combination of options does not scale.

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.

2 participants