-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add Mercury platform. #19
Conversation
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.
Some nits. I haven't yet looked closely at the board itself.
nmigen_boards/mercury.py
Outdated
), | ||
|
||
# ADC over SPI- FPGA is primary. | ||
Resource("adc", 0, |
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.
This should be SPIResources, I think; there's currently no way to override the name, but probably there should be!
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 agree, and I was going to do this until I saw there was no name override.
Looks like you already noticed this, but just to reiterate, SPIResources
does not work for spiserial
because all the SPI pins are reversed by nature of FPGA being the secondary. Perhaps add something to account for this too?
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.
We could add a dir
switch to SPIResources itself, defaulting to "o"
.
nmigen_boards/mercury.py
Outdated
# https://github.com/cr1901/mercpcl | ||
mercpcl = os.environ.get("MERCPCL", "mercpcl") | ||
with products.extract("{}.bin".format(name)) as bitstream_filename: | ||
subprocess.run([mercpcl, bitstream_filename], check=True) |
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.
This should use check_call
instead (I fixed it recently on other boards).
nmigen_boards/mercury.py
Outdated
|
||
sevenseg = [ | ||
Resource("sevenseg", 0, | ||
Subsignal("a", PinsN("13", dir="o", conn=("gpio", 0))), |
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.
This and the two following resources have wrong indentation.
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 accidentally forgot to remove the check=True
argument, so I fixed and forced-pushed. Just FYI.
] | ||
|
||
# The remaining peripherals only make sense w/ the Baseboard installed. | ||
# See: http://www.micro-nova.com/mercury-baseboard/ |
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.
Shouldn't this be one large group of resources called baseboard
?
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.
No, because of the way SRAM works on this board, there are times you want to selectively enable certain baseboard peripherals without enabling others. In particular, only VGA and PS/2 can be enabled while still giving you access to the SRAM.
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 maybe more like baseboard_no_sram
and baseboard_sram
then?
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.
That's a good compromise1, I'll look at the schematic and see which components fit under each. To avoid repetition, I think I should keep the existing Resource
arrays around, but maybe prefix with _
as "internal"?
- Btw, just as an anecdote- even with the baseboard attachment, all of Mercury's GPIO is still broken out, so one can attach their own peripherals. In practice, I've found this to not work at all due to loading issues :P.
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.
To avoid repetition, I think I should keep the existing
Resource
arrays around, but maybe prefix with_
as "internal"?
Yes, that's what I would do.
nmigen_boards/mercury.py
Outdated
|
||
# SRAM and 5V-tolerant I/O share a parallel bus on 200k gate version. | ||
# The SRAM controller needs to take care of switching the bus between | ||
# the two. Meant to be Cat() into one GPIO bus, and combined with gpio_ctl. |
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.
This is... really, really confusing. Just reading this comment I have absolutely no idea how to use it correctly. Perhaps this needs some rethinking?
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.
Put another way:
The gpio_ctl
Resource
shouldn't exist at all. It is only useful when paired with the gpio_sram_bus
Resource
.
gpio_sram_bus
and essentially the whole gpio
connector are electrically connected, so using the SRAM implies giving up the gpio
connector.
The remaining two pins provided by gpio_ctl
are not broken out to any connector. omigen
couldn't handle a peripheral that was only-partially broken-out to a connector. So I added that comment to let ppl know (apparently badly) that "you really can't use gpio_sram_bus
without gpio_ctl
and you should do plat.request
for both and Cat
them into a single Signal
- or better yet, merge them into a single Record
- after the fact.
Can nmigen
handle a only-partially broken-out peripheral as a single Resource
?
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.
Can nmigen handle a only-partially broken-out peripheral as a single
Resource
?
The following Resource
definition appears to "do the correct thing":
gpio_sram_bus = [
Resource("gpio_sram_bus", 0,
Subsignal("ce", PinsN("P3", dir="o")), # Memory chip-enable.
# Called MEM_CEN in schematic.
Subsignal("bussw_oe", Pins("P30N", dir="o")), # 5V tolerant GPIO
# is shared w/ memory using this pin.
# Actual SRAM connections.
Subsignal("a", Pins(""" 1 2 3 4 5 6 7 8 9 10
11 12 13 14 15 16 17 18 19 20""", dir="o",
conn=("gpio", 0))),
# A19 is actually unused- free for GPIO
# 8-bit data bus
Subsignal("d", Pins("21 22 23 24 25 26 27 28", dir="io",
conn=("gpio", 0))),
Subsignal("we", PinsN("29", dir="o", conn=("gpio", 0))),
# "Unused" is only used by GPIO, but is included here for
# consistency.
Subsignal("unused", Pins("30", dir="io", conn=("gpio", 0))),
# Subsignal("oe_n", Pins()), # If OE wasn't tied to ground on
# Mercury, this pin would be here.
Attrs(IOSTANDARD="LVTTL", SLEW="FAST")
)
]
From playing in a REPL:
William@William-THINK MINGW64 ~/src/migen_mercury/baseboard
$ python3
Python 3.7.3 (default, Apr 23 2019, 08:20:20) [GCC 8.3.0 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from nmigen_boards.mercury import *
>>> plat = MercuryPlatform()
>>> plat.add_resources(plat.gpio_sram_bus)
>>> sigs = plat.request("gpio_sram_bus")
>>> sigs
(rec gpio_sram_bus_0 (rec gpio_sram_bus_0__ce o) (rec gpio_sram_bus_0__bussw_oe o) (rec gpio_sram_bus_0__a o) (rec gpio_sram_bus_0__d i o oe) (rec gpio_sram_bus_0__we o) (rec gpio_sram_bus_0__unused i o oe))
From what I can gather, this works because in nmigen
, adding a Resource
is done the same way (plat.add_resource
) regardless of whether said Resource
uses connector pins or not. So it makes sense that mixing and matching Pins
that resolve to connectors and Pins
that resolve directly to FPGA pins works.
I wonder if this was also true in omigen
and I never noticed it?
nmigen_boards/mercury.py
Outdated
p = MercuryPlatform() | ||
p.add_resources(p.leds) | ||
p.build(Blinky("clk50"), do_program=True, | ||
script_after_run=r"""-opt_mode AREA""") |
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.
Is this necessary?
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.
Not at all, just for testing that -opt_mode AREA
works. In omigen
, this was the default for this platform unconditionally.
But I've come around to the idea that in nmigen
, the user should add it on a build-by-build basis.
remove "-opt_mode AREA" test.
Individual resources are now for internal use.
nmigen_boards/mercury.py
Outdated
|
||
# The serial interface and flash memory have a shared SPI bus. | ||
# FPGA is secondary. | ||
SPIResource("spiserial", 0, |
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.
spi_serial
nmigen_boards/mercury.py
Outdated
), | ||
|
||
# ADC over SPI- FPGA is primary. | ||
SPIResource("adc", 0, |
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.
spi_adc
Thanks! |
Self-explanatory, meant to test the newly-added Spartan3A support in
nmigen
(and so I can continue using a board with lots of 5V tolerance until I can buy a Mercury 2 :)).