[spi_device] Add an enable bit for the IP to prevent partial configuration?

[a-will]

For the spi_device IP, should we add an IP-wide enable bit in a CSR? Currently, the spi_device IP is never actually off (power / clock / reset inputs notwithstanding). This means that the spi_device could respond to transactions before the full configuration is ready. Should we gate responses to the host, events on CSRs / interrupts, and the filling of FIFOs until software has a chance to complete configuration?

On the spi_host clock side, this would likely mean sampling the enable bit at the beginning of a transaction and discarding the data if the enable was low.

[a-will]

CC @tjaychen @eunchan @jeoongp

[tjaychen]

ah i like this idea...i think it's better than whatever CSb edge hunting idea I had.
To summarize, would it effectively be that when csb goes low and spi_clock starts, everything behaves "as expected" for a couple of cycles. On the second cycle, you will have discovered whether things are enabled vs not, and then perhaps cycle things back from there?

[tjaychen]

@weicaiyang

[tjaychen]

ah i like this idea...i think it's better than whatever CSb edge hunting idea I had. To summarize, would it effectively be that when csb goes low and spi_clock starts, everything behaves "as expected" for a couple of cycles. On the second cycle, you will have discovered whether things are enabled vs not, and then perhaps cycle things back from there?

A number of FSMs and stuff would probably need to be "reset" etc.

[eunchan]

Should we assign this to "Future Release"? I don't think we have enough time changing the design and review CDC.

[tjaychen]

we might have to quantify the host impact a bit.
Let me ruminate a bit on this offline, since I probably can't put it all in the open.

[eunchan]

I will add an enable bit to CSR (rather than adding CG to SPI_CLK or OR gate on CSb) that blocks cmdparse to activate any datapaths. It would take less effect to the timing.

[andreaskurth]

Triaged for spi_device. Assigning to M2.5 with (existing) Priority:P1 Priority: high because we should clarify with the product team if this is a requirement.

[andreaskurth]

We have to add this enable bit. The proposal is as follows:

Add an enable register field that:

• resets to 1’b0;
• gets 2FF-synced to the SPI clock and there resets with rst_ni & ~csb_i (active low) to 1’b0 (The rationale for the reset condition is that the two modules into which the enable signal in the SPI clock domain gets passed, spi_cmdparse and spi_passthrough, are also reset based on the same condition. Using the same reset signal avoids RDC issues.);
• when 1’b0 in the SPI clock domain, causes a transition to StWait in spi_cmdparse at the end of the command phase (i.e., when the 8th bit is valid);
• when 1’b0 in the SPI clock domain, causes a transition to StFilter in spi_passthrough at the end of the command phase (i.e., when the 8th bit is valid).

Keeping Priority:P1 Priority: high because this requires an RTL change and subsequent DV changes and functional coverage.

[hcallahan-lowrisc]

estimate 20
remaining 2023-03-22 20

[msfschaffner]

It looks like we can reset both spi_device and spi_host via the system reset controls:

opentitan/hw/top_earlgrey/data/top_earlgrey.hjson

Lines 155 to 157 in 00b0568

	{ name: "spi_device", gen: true, type: "top", parent: "lc_src", clk: "io_div4", sw: true }
	{ name: "spi_host0", gen: true, type: "top", parent: "lc_src", clk: "io", sw: true }
	{ name: "spi_host1", gen: true, type: "top", parent: "lc_src", clk: "io_div2", sw: true }

Hence, even if it would be convenient to implement this in the blocks themselves, there is a workaround for this available, hence I think we can deprioritize this to after M2.5.

[andreaskurth]

It looks like we can reset both spi_device and spi_host via the system reset controls

This also resets spi_device's configuration, right? So when you bring it out of reset, how do you prevent it from doing unintended things until it's fully configured? The enable register field was meant to do that

[msfschaffner]

@andreaskurth you are right, this workaround would only partially address the issue at hand.
To that end, I discussed this some more with @moidx and @a-will to gauge whether we need this for M2.5 or not.
We think we can work around all interesting integration use cases for which M2.5 will be used.

Specifically:

• TPM mode: can be modulated via IO bit
• passthrough mode: host can either be reset by OT, or the test can be set up so that OT and the host are properly reset sequenced
• flash mode: we can control the timing during bootstrap modes
• generic mode: may be used during manufacturing tests where timing can be controlled explicitly

So, while this is a good feature to have for M3, we decided to de-prioritize this feature for M2.5 and defer it.

[hcallahan-lowrisc]

Related comment : #14861 (review)

[a-will]

This is likely not necessary. The data sheet will always have a spec requirement to wait some amount of time before transactions are allowed (the "reset delay" or similar). The response to such activity is considered undefined behavior.

We can sweep this sort of configuration need into that time requirement.

[a-will]

I'm going to close this, since that time requirement is always present. However, if someone wants to implement this to improve robustness in the future, then we do have a "Disabled" mode now that generic mode is gone. The bits would need to be held steady and sampled at the right moment to allow the transition to the SPI domain, in addition to ensuring the atomicity of SPI transactions.