[chip] Convert Edge-triggered status interrupt to Level

[eunchan]

• Revise the comportability spec
  • Define event and status. Event is one-time trigger, and status is the current HW status (e.g: FIFO empty)
  • Draw two waveforms (event: set -> SW clears) (status: asserted, HW condition alleviated then status drops)
• [uart] Convert edge based interrupt to level  #16693
• [spi_device] Change of interrupt types to status #21207
• [hmac] Change fifo_empty interrupt from event to status type #21206
• [kmac] Change fifo_empty interrupt from event to status type #21049
• [spi_host] Change of interrupt types to status #21208
• [i2c] Change of interrupt types to status #21209
• [usbdev] Change of interrupt types to status #21210
• [flash_ctrl] Change of interrupt types to status #21211

CC: @weicaiyang @a-will

[tjaychen]

i think if there aren't any identified issues, it's okay to not change.
I think @weicaiyang point on SPI device is more that we don't control the host timing, so it is difficult to flag the corner cases.
I think flash has a few like that too, i need to look closely.

[tjaychen]

i split the uart one to a separate issue since uart has already passed D3...

[eunchan]

Changing this to backlog as we decided not to change if the current interrupts work.

[GregAC]

Triaged for flash_ctrl, agreed that this isn't something we need to change.

[andreaskurth]

Triaged for spi_device, agreed

[GregAC]

Triaged for uart, same reasoning as above

[ballifatih]

Triaged for kmac, agreed, not necessary for M2.5.

[andreaskurth]

Thanks @ballifatih.

@cindychip: WDYT for hmac (since you've triaged that block)? If you agree this is not necessary for this release (none of the currently planned milestones), we can conclude the discussion

[msfschaffner]

Carried over from #12989:

Wishlist for status-type interrupts:

• spi_device: generic_rx_full, generic_rx_watermark, generic_tx_watermark, readbuf_watermark,
  upload_cmdfifo_not_empty, upload_payload_not_empty
• i2c: fmt_threshold and rx_threshold (as well as future tx_threshold and acq_threshold)
• spi_host: It seems that six different sources are combined into one event-type interrupt, spi_event. Four of them are clearly status type (RXFULL, TXEMPTY, RXWM, TXWM), the other two READY and IDLE could also work as status-type, so I would like to ask that the entire spi_event interrupt become status-type. (Already, the description of e.g. RXFULL makes it sound as if status-type logic applies, "To prevent the reassertion of this interrupt, read more data from the RX FIFO, or increase CONTROL.RX_WATERMARK.")

Thanks for mentioning it, further wishes:

• UART: tx_watermark and rx_watermark would be better as status-type, tx_empty should remain event-type
• USB: pkt_received, pkt_sent, av_empty, and rx_full all come with explanations that the underlying condition needs to be addressed, before the interrupt can be cleared, and that it will otherwise re-assert. I would be more natural to make these status-type, as that is basically how they already work.

[vogelpi]

@msfschaffner , I am trying to assess whether we should make changes for KMAC as well and if yes for which KMAC interrupts:

• kmac_done : signals when the absorption has completed, event seems fine for this.
• kmac_err: signals when an error has occured, event seems fine for this.
• fifo_empty: indicates if the message FIFO is empty, status seems more sensible for this one. It has also been proposed by Eli initially. However, in the list above there are cases where event seems for preferrerable (tx_empty for UART) and cases where status is preferred (TXEMPTY for SPI_HOST). Please let me know if I missed something.

Independently, I am now factoring the DD/DV work KMAC out into a seperate issue (#21049).

[a-will]

I actually don't understand why UART's tx_empty would be an event type. What purpose does that have? Why would someone want notifications for "The FIFO went empty sometime after you last cleared the bit" and not "The FIFO is currently empty"?

Careful order of operations can make the former imply the latter, but personally, I'm not a fan of potential race conditions at interfaces. ;)

That said, it probably doesn't make much difference. The status-type watermark interrupt is better for notifying about available space for sending more data. tx_empty is probably only useful as a fence, if you want to know that a particular discrete message finished sending, for timing purposes or otherwise.

[vogelpi]

I thought the preference stated by others in previous comments might be relevant for UART only due to the much slower timing. But who knows. Thanks for sharing your opinion, I think we are both on the same page. :-)

[andreaskurth]

For HMAC, I concur with @vogelpi's suggestion for KMAC:

• hmac_done: is fine as event
• fifo_empty: should be changed to status
• hmac_err: is fine as event

[jdonjdon]

@msfschaffner
For flash_ctrl, interrupt based data transaction mechanism has never been used.
(Tested only in the block level and which works fine).
I don't see any reason we take the risk to modify this IP as @tjaychen commented earlier
(#15378 (comment)).

[msfschaffner]

@vogelpi / @andreaskurth that sounds good for HMAC / KMAC. I have split out the HMAC one as well here #21206.

[msfschaffner]

@jesultra was there a specific reason to keep the tx_empty interrupt for UART as event instead of converting to status?

[msfschaffner]

I have split the tasks out into individual issues that are associated with the individual blocks.
Hence I am closing this issue since the effort is now tracked elsewhere.

CC @johngt

[jettr]

I have another interrupt that I think should be status instead of event: intr_state on the System Reset Controller. This should be a status based on the state of the combo_intr_status and key_intr_status registers (which should remain event-based though). Otherwise there are race conditions trying to read/clear those three registers correctly. Thoughts?

[andreaskurth]

Thanks for this suggestion; reopening for @msfschaffner to assess.

[msfschaffner]

I think that makes sense. There is a similar one in adc_ctrl which does not make any sense to keep edge-based, since it forwards a collated set of interrupts:

opentitan/hw/ip/adc_ctrl/data/adc_ctrl.hjson

Lines 353 to 371 in 7e196e6

	{ name: "adc_intr_status",
	desc: "Debug cable internal status",
	swaccess: "rw1c",
	hwaccess: "hrw",
	resval: "0",
	tags: [ // the value of these regs is determined by the
	// value on the pins, hence it cannot be predicted.
	"excl:CsrNonInitTests:CsrExclCheck"],
	fields: [
	{ bits: "NumAdcFilter-1:0",
	name: "filter_match",
	desc: "0: filter condition is not met; 1: filter condition is met",
	}
	{ bits: "NumAdcFilter:NumAdcFilter",
	name: "oneshot",
	desc: "0: oneshot sample is not done ; 1: oneshot sample is done",
	}
	]
	}

I will convert these when I open up adc_ctrl.
Created an issue to track here: #21832