Please Review: SPI DO OENb signal source

[dlmiles]

https://github.com/efabless/caravel_mgmt_soc_litex/blob/43d0ce33d331ee73d9dcebe197c6ce4da5909ecc/verilog/rtl/mgmt_core.v#L1774C31-L1774C31

This like appears to connect the SPI master controller Data Out Output Enable (active low) to the INVERTED signal of the SPI CS (active low) as a signal source. But surely with SPI, DO is only active when CS is active and they are both active low signals. So with this inversion in place it prevents the DO signal from making it to the external pin.

In my Arty-A7 port it was necessary to remove the tilde ~ character.

Maybe this is a left-over from GF180 which may have used Output Enable (active high) signals ? but sky130_fd_io_gpiov2 cells have Output Enable (active low) signals ?

Maybe as a workaround it is possible to reconfigure the gpio_control for the SPI CS PIN as necessary to achieve pull-down or pull-up, instead on expecting to be able to use the SPI master controller hardware device for this purpose, which I believe is the design intention.

EDIT: Sorry I mistyped the last paragraph and spoke in terms of CS and not DO concerning the use of gpio_control to workaround. But RTE has understood matters in his comment below. It is not the CS output that could be problematic, it is DO.

[RTimothyEdwards]

@shalan @mo-hosni @marwaneltoukhy : Please review.
The comment does not contain a line number but I assume it refers to mgmt_core.v line 1774:

assign spi_sdoenb = (~spi_cs_n);

On the face of it, this is incorrect. It's possible that the SPI master testbench is written wrong by forcing GPIO[35] to be an output and thereby overriding the SDO enb signal, which otherwise should be driving the oeb on that pin. I see that the C code for the cocotb spi_master testbench has GPIO[35] set to GPIO_MODE_MGMT_STD_BIDIRECTIONAL, which sounds right, but isn't; the actual configuration value needed to properly exercise the SDO is 0x1803 (which doesn't have a corresponding named definition). If the testbench configuration of GPIO[35] is set to 0x1803, you will presumably find that the testbench no longer passes because of mgmt_core.v line 1774 above.

The corrected mgmt_core.v line 1174 should read:

assign spi_sdoenb = spi_cs_n;

(@azwefabless , FYI)

[RTimothyEdwards]

@dlmiles: Since the caravel GPIO pads have a very specific control interface that is presumably nothing like the GPIO on the Arty, this can be worked around on Caravel by forcing the pin to be an output always, which works if the SPI SDO line is not intended to be shared between multiple pins (normally that isn't done on the SPI master side anyway).

[dlmiles]

You have the correct line. It is part of the github link url "L1774" if you know how to decode.

Sure the Arty supports IN, OUT, OEB, INP_DIS, none of the other modes for ANalogue or DMmode are relevant, even if some of the DMmode settings could be implemented, it is not clear for what benefit if your FPGA is already attached to a dev board (such as Arty).

FWIW I found the automatic CS mode (provide by SPI master hardware) to not be useful, it would make CS active during 8-bit of Data Out and de-assert itself immediately (before any reply). It is not clear to me why that mode even exists to broadcast 8-bit commands into the SPI void (the logic/area could be better used for something else, like some UART divisor bits :) to provide core_clock/baud options.

Having software manually set the CS bit (0x10000) around the SPI transaction worked as expected. I was only testing with PmodSF3 (32MB serial NOR flash memory N25Q)