Moved the memories spi block back into the pinmux. #326
Conversation
This will make it slightly easier to lock down a spi device in software. This should also fix the problem found by adrian where: > Combining CIPO lines directly without regard to Chip Selects > is risky, and in particular the line `appspi_d1` from the > flash is tri-stated without a pullup resistor on the FPGA > board. Also, added microSD card signals to pave the way for the microSD card DPI model he has built.
HU90m
requested review from
marnovandermaas,
alees24 and
GregAC
and removed request for
marnovandermaas and
alees24
HU90m
force-pushed
the
pinmux
branch
2 times, most recently
from
812fd0b to
a8849f9
Compare
|
All tests passing apart from the SPI Flash Pinmux test |
1. The address of the SPI connected to flash has changes. 2. There are now three SPI devices going through the pinmux, so the pinmux driver has changed.
1. Moved rs485 to be with the other UARTs in the register map.
2. Removed the SPI suffixes from the raspberry pi hat. They were
inconsistent.
3. Changed the Pmod{0,1} pin numbering to match the Pmod standard
physical pins as oppose to the sonata schematic.
| @@ -291,6 +291,7 @@ set_property -dict { PACKAGE_PIN C10 IOSTANDARD LVCMOS33 } [get_ports appspi_d1] | |||
| set_property -dict { PACKAGE_PIN A10 IOSTANDARD LVCMOS33 } [get_ports appspi_d2] | |||
| set_property -dict { PACKAGE_PIN A9 IOSTANDARD LVCMOS33 } [get_ports appspi_d3] | |||
| set_property -dict { PACKAGE_PIN D10 IOSTANDARD LVCMOS33 } [get_ports appspi_cs] | |||
| set_property PULLTYPE PULLUP [get_ports appspi_d1] | |||
There was a problem hiding this comment.
I'm wondering if we should add pull ups to all of the CIPO lines. If it makes sense here it makes sense everywhere.
There was a problem hiding this comment.
The microsd card lines appear to be pulled high on the board. Ethernet MAC chip (KSZ8851SNL/SNLI) is high impedance by default, so may make sense to pull high. I don't know what the behavior of the LCD is. And the LCD doesn't have a CIPO line.
What would you like me to do for this PR?
There was a problem hiding this comment.
@alees24 🪣 (passing the buck on the ethmac CIPO)
| name = "rs485_tx" | ||
| block_ios = [{ block = "uart", instance = 2, io = "tx" }] | ||
| no_default_out = true | ||
|
|
There was a problem hiding this comment.
I did put them at the end intentionally to avoid altering other mappings. In particular the UART 2 RX input selector. Previously RS485 sat at the end so anything else that wanted to mux a pin to the UART 2 RX input wouldn't now need to change which input it selected.
The real issue here was lack of constants for the actual selectors being generated by the pinmux generator, though maybe we do have that and I haven't noticed!
Certainly this change will need a fix in the rs485 check at least.
There was a problem hiding this comment.
I appreciate wanting to avoid breaking other mappings, but I believe the RS485 check is the only software that is affected by this change. So I guess it's a question of what's preferred.
What register mapping do you prefer?
| // Tie flash wp_n and hold_n to 1 as they're active low and we don't need either signal | ||
| assign appspi_d2 = 1'b1; | ||
| assign appspi_d3 = 1'b1; | ||
| wire appspi_clk = out_to_pins[OUT_PIN_APPSPI_CLK]; |
There was a problem hiding this comment.
Should we not use the assign statement here?
There was a problem hiding this comment.
My understanding is that
// when a is a net, it's a continuous assignment
wire a = b;
// equivalent to
wire a;
assign a = b;
// whereas when a is a variable, it's an initialisation assignment
var a = b;I think it's generally less bug prone to useful to have the assignment with the definition in simple cases.
There was a problem hiding this comment.
Ok, I guess I don't really understand why we're using wires here in the first place, but let's leave it for now.
There was a problem hiding this comment.
For reference I'm really not a fan of this style, exactly because you get different semantics when this changes between wire and logic. Far better to just have a consistent don't assign with declarations rules.
|
Thank you both for the very prompt reviews |
This gives a little more flexibility in software compartmentalization and should also solve the problem found in #323