opentitan
opentitan copied to clipboard
lowRISC
[uart] Convert edge based interrupt to level
Split out from #15378:
- [ ] tx_watermark
- [ ] rx_watermark
- [ ] tx_empty should remain event-type
do you guys think this should be a backlog issue? It would be very nice to fix, but UART is pretty close to V3 already...
actually...an interrupt change from edge triggered to status might be considered a major change if semantic versioning were to go through.. @msfschaffner
I'm OK with either way. If we want to fix it, it's nice to fix all the watermarks, so that we have a consistent behavior across the project.
If it's easy to fix I would do it now, otherwise we will have to open this up again. And I have a feeling that the status kind is more amenable to SW drivers...
CC @a-will @timothytrippel
Given comments in https://github.com/lowRISC/opentitan/issues/15378 this isn't needed for M2.5
Total effort upped to 1.5 days. PR has been done https://github.com/lowRISC/opentitan/pull/21632 though still needs review and need to check if there's anything to adjust in DIFs/top-level tests.
The two watermark interrupt moving to status SGTM.
For the empty interrupt, it would be nice for there to be consistency across other empty interrupts, e.g. fifo_empty. If fifo_empty is status, then it seems like uart's tx_empty should be status as well (see also #15378)
Yes, it was me who originally requested that tx_empty were to remain event type. But thinking about it, Jett is right that it is more consistent to have tx_empty be a status-type interrupt, to match other IPS and as the name suggests that it is a condition, not an event.
Status-type is also how e.g. UART "TX fifo empty" interrupt on STM32L5 (HyperDebug EC) works. That chip has a separate event-type interrupt called "Transmission complete", which is triggered when not only the FIFO is empty, but the shift register finishes the last stop bit while the FIFO is empty. Such an interrupt could be useful if software wants to e.g. reset the chip after transmitting a message on the UART. For such a use case, TX FIFO empty (whether status or event) would be insufficient, as the UART shift register would still be active, when it fires.
So after thinking about it, I would also like tx_empty to be changed into a status-type interrupt. And additional nice-to-have feature request is to have a "transmission complete" event-type interrupt.
PR here to switch tx_empty to a status type: https://github.com/lowRISC/opentitan/pull/21844
@jesultra @jettr it turns out the existing tx_empty was actually the "transmission complete" interrupt you describe it triggers when the FIFO is empty and the TX is idle. From a comment in the RTL this was done to avoid an immediate tx_empty interrupt when you write a single byte to the FIFO (as it will be immediately popped off and transmitted).
We could alter this to a 'proper' tx_empty that only cares about the state of the FIFO and rename the existing one to tx_done or similar or just leave it as is. I'd prefer the latter as it's less change. What are your thoughts?
In the case you described:
From a comment in the RTL this was done to avoid an immediate tx_empty interrupt when you write a single byte to the FIFO (as it will be immediately popped off and transmitted).
Wouldn't either implementation of tx_empty as 1) a true status interrupt on fifo state or 2) an event based interrupt on fifo state and and tx idle cause the interrupt to fire? The currently implementation would just fire with a slight delay.
If my understanding it correct, I think it would still be preferable to make tx_empty a true status-type interrupt, and if we could also keep the existing implementation and rename it to tx_complete/tx_done event type would be nice to have as well.
Wouldn't either implementation of tx_empty as 1) a true status interrupt on fifo state or 2) an event based interrupt on fifo state and and tx idle cause the interrupt to fire? The currently implementation would just fire with a slight delay.
The full comment is here:
https://github.com/lowRISC/opentitan/blob/051b3a54d54e88f52d92fe5c59a3f52d675d8026/hw/ip/uart/rtl/uart_core.sv#L320-L330
The issue it's seeking to solve is the copy loop will write one byte at a time, so when you write several bytes of data you'll get an immediate tx_empty IRQ after the first byte write before the software has a chance to write anything further.
As we're out of time for RTL changes for M2 now I'm going to leave things as is which means:
rx_watermark/tx_watermarkare now status type interruptstx_emptyremains an event type however it's behaviour is actually the "transmission complete" behaviour described above where it fires when both the FIFO is empty and the TX is idle
There's scope for small changes beyond M2 so we can look at renaming tx_empty to tx_done and introducing a 'proper' tx_empty at that point. Note the tx_empty naming is really just a matter of documentation so it doesn't really matter what it's called in RTL signals.
@jettr @jesultra @msfschaffner please note the above.
The issue it's seeking to solve is the copy loop will write one byte at a time, so when you write several bytes of data you'll get an immediate tx_empty IRQ after the first byte write before the software has a chance to write anything further.
The FW code would want to disable a real status-based tx_empty interrupt while loading up the FIFO until, then enable the interrupt afterwards if the FW was interested in doing something once it was empty. If it were a status-based interrupt, then FW would have to manage the enable/disable state of the itnerrupt anyway
There's scope for small changes beyond M2 so we can look at renaming
tx_emptytotx_doneand introducing a 'proper'tx_emptyat that point. Note thetx_emptynaming is really just a matter of documentation so it doesn't really matter what it's called in RTL signals.@jettr @jesultra @msfschaffner please note the above.
Understood. Let's rename tx_empty to tx_done in documentation then, and it should be okay.
PR for the above change is here: https://github.com/lowRISC/opentitan/pull/23409
