[uart] Spec/ Design change

[eunchan]

A SW person raises an issue about the tx_watermark behavior isn't optimal for the software usage case. It makes more sense if UART creates an tx_watermark interrupt when its TX FIFO drops the level below to the watermark level.

CC: @weicaiyang

[tjaychen]

ah good catch, i didn't realize today's implementation is >= So i'm assuming the feedback is that software wants to know when the FIFO has been drained a certain amount so that they can fill in more?

[eunchan]

ah good catch, i didn't realize today's implementation is >= So i'm assuming the feedback is that software wants to know when the FIFO has been drained a certain amount so that they can fill in more?

Exactly.

[eunchan]

ah good catch, i didn't realize today's implementation is >=

I remember @weicaiyang raised this issue (spec and the implementation weren't same), and I blindly revised the spec rather digging out the issue deeper. My bad... :)

[phil-levis-google]

I'm the SW person @eunchan mentions.

The desired behavior is that hardware informs software when it needs to take action. In the case of RX, this happens when the FIFO is too full -- software needs to read it or there will be an overflow and bytes will be lost. The RX interrupt behavior seems correct for this (except for one detail noted in another message below). For TX, this happens when the FIFO is close to empty -- software needs to put more bytes into the FIFO or the TX will fall idle. Specifically, it is probably best if the TX FIFO interrupt is when it falls below the watermark; a watermark of 1 means you receive an interrupt when the TX FIFO is empty, while a watermark of 16 means you receive an interrupt when the TXFIFO has 15 bytes in it.

In the best case, there is also an "empty" interrupt. This is important to know when everything has been transmitted. The watermark interrupt is intended to allow software to keep the TXFIFO full without having to handle per-byte interrupts (lower software/energy overhead for long transfers). The empty interrupt is for software to know when the operation is complete and so the buffer can be reclaimed.

You can get away with only having a watermark interrupt, and setting the watermark to 1 to detect completion, but this requires doing some logic to guess how full the FIFO is in order to figure out where to set the watermark. E.g., suppose software does this:

make a 3 byte transfer make a 60 byte transfer make a 19 byte transfer

The 3 byte transfer would just like to know when the transfer completes. The 60 byte transfer, however, would like to also like to know when to put more data into the FIFO, until its last chunk, at which point it wants to know about completion. Then, if it wants to know about completion, but a new 19 byte transfer comes in which can't fit in the FIFO, we need to know about the watermark again. Software could track this, and based on whether the FIFO is full set the watermark to be 1 or something larger, but if there's an error and the watermark is high when it should be 1, we could hang forever without (assuming an edge triggered interrupt).

The simplest way to handle this is to receive an interrupt both when the FIFO falls empty as well as when it falls below a watermark. The logic for the empty FIFO interrupt is "signal transfer is complete" while the logic for the watermark interrupt is "push more data if it is available".

Most modern microcontrollers I've played with do away with the watermark because they rely on DMA rather than FIFO. But if there's a FIFO having a watermark is very helpful.

[phil-levis-google]

On follow-up. While it's important that the TX watermark trigger when the TXFIFO falls below the watermark, so a watermark of 1 will trigger at 0, it's equally important that the RX watermark trigger when the RXFIFO rises to be equal to or greater than the watermark. This is so that if there is a watermark of 1 and a single byte is received, it will not remain pending in the FIFO for an arbitrarily long period until a second byte is received. The watermarks need to be able to detect the 0->1 case in TX and the 0->1 case in RX.

[sjgitty]

Thanks for the feedback, @phil-levis-google, more SW eyes are certainly welcome. This seems like the right implementation. Is there another request for a second (empty) interrupt, or is one (watermark) sufficient?

On Tue, Dec 3, 2019 at 1:21 PM phil-levis-google notifications@github.com wrote:

On follow-up. While it's important that the TX watermark trigger when the TXFIFO falls below the watermark, so a watermark of 1 will trigger at 0, it's equally important that the RX watermark trigger when the RXFIFO rises to be equal to or greater than the watermark. This is so that if there is a watermark of 1 and a single byte is received, it will not remain pending in the FIFO for an arbitrarily long period until a second byte is received. The watermarks need to be able to detect the 0->1 case in TX and the 0->1 case in RX.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/1132?email_source=notifications&email_token=AJZQKVKTFJUPQYZAAHDXGGTQW3ET7A5CNFSM4JU5UM6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF23K5A#issuecomment-561362292, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJZQKVJVZHKR4M2NFHDKPCTQW3ET7ANCNFSM4JU5UM6A .

[phil-levis]

I'd suggest changing the TX overflow interrupt to be an empty interrupt: a TX overflow interrupt is not useful. Both the RX and TX paths have two interrupts: RX for watermark and overflow, TX for watermark and empty. Should I put this in a separate issue?

[eunchan]

No, we can do it in this issue. On Tue, Dec 03, 2019 at 10:16:03PM -0800, Philip Levis wrote:

I'd suggest changing the TX overflow interrupt to be an empty interrupt: a TX overflow interrupt is not useful. Both the RX and TX paths have two interrupts: RX for watermark and overflow, TX for watermark and empty. Should I put this in a separate issue?

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/lowRISC/opentitan/issues/1132#issuecomment-561494708

[tjaychen]

yeah i agree on tx_overflow -> tx_empty.

On Wed, Dec 4, 2019 at 8:20 AM Eunchan Kim notifications@github.com wrote:

No, we can do it in this issue. On Tue, Dec 03, 2019 at 10:16:03PM -0800, Philip Levis wrote:

I'd suggest changing the TX overflow interrupt to be an empty interrupt: a TX overflow interrupt is not useful. Both the RX and TX paths have two interrupts: RX for watermark and overflow, TX for watermark and empty. Should I put this in a separate issue?

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/lowRISC/opentitan/issues/1132#issuecomment-561494708

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/1132?email_source=notifications&email_token=AAH2RSUL7UH44W22452XCT3QW7KD3A5CNFSM4JU5UM6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF5SMDI#issuecomment-561718797, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2RSRG3WTNWB7Q5DWC5N3QW7KD3ANCNFSM4JU5UM6A .

[tjaychen]

i can take this one.

[eunchan]

Hey Tim,

Thanks for willing to take the task, appreciated it! If you didn't start, I can do this in this week. Let me know.

Eunchan

On Thu, Dec 19, 2019 at 08:35:13PM -0800, tjaychen wrote:

i can take this one.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/lowRISC/opentitan/issues/1132#issuecomment-567783837

[tjaychen]

No problem! I know you have a ton of things to do already, so I figured might as well since I was looking at interrupt related things with Alistair.

On Mon, Dec 23, 2019, 08:20 Eunchan Kim notifications@github.com wrote:

Hey Tim,

Thanks for willing to take the task, appreciated it! If you didn't start, I can do this in this week. Let me know.

Eunchan

On Thu, Dec 19, 2019 at 08:35:13PM -0800, tjaychen wrote:

i can take this one.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/lowRISC/opentitan/issues/1132#issuecomment-567783837

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/1132?email_source=notifications&email_token=AAH2RSWRLZIN5LJ4W7T3ERDQ2DQLTA5CNFSM4JU5UM6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHROCUQ#issuecomment-568516946, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2RSXW54USDAFBOL56EETQ2DQLTANCNFSM4JU5UM6A .