[uart,rtl] Increase buffer size to 128

[GregAC]

This a draft as it's incomplete but creating the PR now for visibility.

There are regression failures with this change I have yet to look into plus we need to discuss the appropriate size to move to and what we switch watermark levels to.

Fixes #16549

[GregAC]

@jesultra this increases UART buffer sizes to 64 from 32. Can you confirm:

• If 64 is the appropriate buffer size
• If the new watermarking levels are appropriate, see table below

TX/RXILVL setting	RX watermark level	TX watermark level
0	1	4
1	8	8
2	16	16
3	32	32
4	62	-

[johngt]

Based on conversations - the understanding is to move to 128 bytes and increase watermark bits; 3 to 4.

[GregAC]

This is now ready to go. Following discussion we've decided to expand the size of the watermark level fields and just provide more levels to deal with the expanded FIFO size. The new settings are:

TX/RXILVL setting	RX watermark level	TX watermark level
0	1	2
1	4	4
2	8	8
3	16	16
4	32	32
5	64	64
6	126	-

I note in the DIF we have a single enum for both the TX and RX settings which is a little awkward as the 0 level is different for TX and RX. The current enum looks like this:

typedef enum dif_uart_watermark {
  /**
   * Indicates a one-byte watermark.
   */
  kDifUartWatermarkByte1 = 0,

Which isn't strictly correct. Something to fix but not something I want to gate this PR on. We can create a separate issue for it.

@msfschaffner @jesultra @cfrantz any comments? In particular are we happy with the watermarking levels?

[jesultra]

The watermarking levels and the FIFO size of 128 bytes fully cover the needs of ChromeOS.

[cfrantz]

I note in the DIF we have a single enum for both the TX and RX settings which is a little awkward as the 0 level is different for TX and RX.

We already have a discrepancy like this for the current watermark levels, so I don't think its a big problem. Out of curiosity, how difficult would it be to make the TX and RX watermark levels symmetric such that the levels are always 2**n up to the last one which is 2**n - k (where k currently is 2).

[GregAC]

I note in the DIF we have a single enum for both the TX and RX settings which is a little awkward as the 0 level is different for TX and RX.

We already have a discrepancy like this for the current watermark levels, so I don't think its a big problem. Out of curiosity, how difficult would it be to make the TX and RX watermark levels symmetric such that the levels are always 2**n up to the last one which is 2**n - k (where k currently is 2).

Trivial I was just going with what was already there but extended for 128 bytes.

To confirm you want settings like this?

TX/RXILVL setting	RX watermark level	TX watermark level
0	1	1
1	2	2
2	4	4
3	8	8
4	16	16
5	32	32
6	64	64
7	126	-

We can of course have a 126 level for the TX as well but maybe doesn't make a huge amount of sense (RX FIFO almost full obviously useful, TX FIFO just beginning to empty less so).

[cfrantz]

To confirm you want settings like this?

[snip]
We can of course have a 126 level for the TX as well but maybe doesn't make a huge amount of sense (RX FIFO almost full obviously useful, TX FIFO just beginning to empty less so).

Agreed about 126 for TX. I also think the TX watermark level of 1 is dubious, but I'm giving consideration to the value of symmetry in the programming interface.

As long is it isn't burdensome to implement, your previous table (1-126 for RX, 1-64 for TX) looks good to me.

[msfschaffner]

Thanks @GregAC - I am fine with these changes.

Regarding the levels, I have a slight preference towards symmetry as well.

[marnovandermaas]

LGTM, thanks Greg!

[marnovandermaas]

Rerunning failed CI tests.

[GregAC]

The watermark changes proved to be fiddlier than first expected as it uncovered some testbench bugs. These are fixed in the second commit of this PR.

Issue from CI failure has also been fixed.

@jdonjdon @msfschaffner would you mind taking a look at the testbench changes?

[msfschaffner]

LGTM