UART only works for 8N1; API exposes functions that mishandle UARTLCR

[MustBeArt]

The documented Hardware UART API exposes useful-sounding functions such as uart_set_baudrate() and uart_set_format() (and others) that write to the UARTLCR registers. This can only be done safely if the UART is not enabled, and then only if the UART is not currently busy transmitting or receiving a character, as documented in ARM's PrimeCell UART (PL011) Technical Reference Manual, in the section on the UARTLCR_H register. These functions make no attempt to ensure this is the case, and there is no warning in the API documentation.

In fact there is no safe way to call these functions through the API, because the API does not expose the UART enable separately, but only through uart_init() and uart_deinit(). The programmer can test the enable with uart_is_enabled(), but cannot control it. If the programmer tries to comply with the restriction on writing UARTLCR by first calling uart_deinit(), then calling (say) uart_set_format(), and finally calling uart_init(), their work is immediately undone by uart_init(). The uart_init() functions HARD CODES the format value to 8 bits, 1 stop bit, and no parity! The result is that any attempt to use word size, stop bits, or parity values other than 8, 1, and none is doomed to failure.

Changing baud rate on the fly (by just calling uart_set_baudrate()) does seem to work empirically, but it still violates the requirements set out in the PL011 manual, so there may be cases I didn't try where it fails.

Changing the stop bits, word width, or parity on the fly (by just calling uart_set_format()) seems to work sometimes and fail sometimes. My current project requires changing all those values quite frequently (to exercise another device's serial port implementation). Sometimes it transmits with the wrong parameters, and other times it fails to transmit at all after a parameter change.

I did not investigate whether other settings in UARTLCR work or not. FIFO enable, for instance, is in UARTLCR_H, and uart_init() has a hard-coded value for that one, too. However, uart_init() erroneously sets the FIFO enable bit AFTER enabling the UART, so it is always in violation of the rule from the PL011 manual. The API function for this, uart_set_fifo_enabled(), writes UARTLCR_H without any checks, just as uart_set_format() does.

It's great that the API presents a simplified model of the UART, hiding all these weird rules from the programmer. I'm not suggesting that more of the details be exposed. In fact, I'd remove the uart_is_enabled() function. I'm suggesting that the implementation of the API should do the necessary magic to make the simplified API work as expected, reliably. It should document any weird side effects caused by the magic (such as: the UART goes dead briefly to change parameters). The typical user should be able to get reliable performance out of the UART without worrying about these details.

The Linux kernel has a surprisingly complicated-looking driver for PL011 UARTs, written originally by ARM and updated by other ARM-licensed manufacturers. It might be valuable to go through that driver and see if there are other hard-learned tricks that need to be included in the Pico SDK's UART APIs.

[daveythacher]

There is an issue in uart_init possibly which is related to this. It enables the module then attempts to enable the FIFO which is forbidden in PrimeCell UART manual. Loopback logic may not be supported by SDK. See this conversation for more information: https://www.raspberrypi.org/forums/viewtopic.php?f=144&t=318787

Recommend to changing this to Update SDK to implement PrimeCell UART. Also RP2040 datasheet lacks most of this, which is very strange.

[daveythacher]

Hardware_uart may need to be refactored. It looks like it currently only supports what is needed by CMake printf.

[kilograham]

what is "CMake printf"?!

[daveythacher]

PICO_STDIO_UART, PICO_UART_DEFAULT_CRLF, PICO_DEFAULT_UART, PICO_DEFAULT_UART_BAUD_RATE, etc.
This
https://raspberrypi.github.io/pico-sdk-doxygen/group__pico__stdio.html
Looks like this:
https://raspberrypi.github.io/pico-sdk-doxygen/group__hardware__uart.html
Not like PL011

[Wren6991]

that write to the UARTLCR registers. This can only be done safely if the UART is not enabled, and then only if the UART is not currently busy transmitting or receiving a character, as documented in ARM's PrimeCell UART (PL011) Technical Reference Manual, in the section on the UARTLCR_H register.

Thanks, this is definitely a bug. Hopefully we can keep the fixes cleanly contained on the implementation side.

[Wren6991]

There is a WIP pull request, currently only worth discussing, not running.

I'd like to preserve the current API. There is code added to enable/disable the UART around LCR updates, which may lead to loss of data, but this isn't such a big deal as different UART data formats are mutually unintelligible anyway.

Unfortunately the approach is a bit of a hack -- see comments at

pico-sdk/src/rp2_common/hardware_uart/uart.c

Lines 73 to 92 in 25b9894

	// Notes from PL011 reference manual:
	//
	// - Before writing LCR, must disable UART and wait for current TX + RX
	// char to finish
	//
	// - There is a BUSY flag which waits for the current TX char, but this is
	// OR'd with TX FIFO !FULL, so not usable when FIFOs are enabled and
	// potentially nonempty
	//
	// - FIFOs can't be set to disabled whilst a character is in progress
	// (else "FIFO integrity is not guaranteed")
	//
	// Combination of these means there is no general way to halt and poll for
	// end of TX character, if FIFOs may be enabled. Either way, there is no
	// way to poll for end of RX character.
	//
	// So... we're going to insert a 15 baud period delay before changing
	// settings (comfortably higher than max data + start + stop + parity).
	// Anything else would require API changes to permit a non-enabled UART
	// state after init() where settings can be changed safely.

. The PL011 doesn't seem to have the necessary status bits for you to poll in order to wait for the current TX/RX characters to complete like it asks you to do. This code just adds a 15 baud period busy wait after disabling the UART, to make sure in-progress characters have finished.

I did have a look at the kernel driver but it's odd, doesn't seem to line up with the information in the reference manual. I have no idea where the 10-PCLK delay in pl011_set_termios comes from for example.

[daveythacher]

I'd like to preserve the current API. There is code added to enable/disable the UART around LCR updates, which may lead to loss of data, but this isn't such a big deal as different UART data formats are mutually unintelligible anyway.

The PL011 doesn't seem to have the necessary status bits for you to poll in order to wait for the current TX/RX characters to complete like it asks you to do. This code just adds a 15 baud period busy wait after disabling the UART, to make sure in-progress characters have finished.

This is not true. This UART module is complete with busy and FIFO flags. You can block. Note you will need to block on DMA? There needs to be warnings in the documentation for hardware_uart.

Taking enable out of uart_init and creating uart_enable is possible. However you could also create structure for uart_init. This is kind of bad. However baud change will now block and call deinit update current struct then proceed with init. Structure will need to be preserved.

MIcrochip's PLIB HAL is known for creating an enable and init routine which allowed reconfiguration then locking it in with enable. That is another approach here. Init would do most of the initialization but a few other things were also possible. This will need to be documented. Avoiding this for this issue here is possible with existing API.

Edit: Continued here - #556 (comment)

[Wren6991]

This is not true. This UART module is complete with busy and FIFO flags. You can block. Note you will need to block on DMA? There needs to be warnings in the documentation for hardware_uart.

There is a TX BUSY flag but no RX BUSY. Even if you poll for TX to completely drain, an RX can begin arbitrarily close to the point where you disable the UART, and you have no flag to precisely wait this out. You can't modify the LCR whilst that RX is still in progress. What am I missing?

[daveythacher]

This is not true. This UART module is complete with busy and FIFO flags. You can block. Note you will need to block on DMA? There needs to be warnings in the documentation for hardware_uart.

There is a TX BUSY flag but no RX BUSY. Even if you poll for TX to completely drain, an RX can begin arbitrarily close to the point where you disable the UART, and you have no flag to precisely wait this out. You can't modify the LCR whilst that RX is still in progress. What am I missing?

Application logic problem. UART is not like SPI and I2C in that it lacks state and is async like. The SDK does not have enough information but does provide a means to solve this. I would recommend ignoring this. It is a both a valid and invalid problem.

The solution of blocking for it to arrive is potentially valid. If the application need instant frequency change after last byte this could be justified. However I think you could also assume that it is forced to be idle at the time of the call. The issue with delay is potentially overhead. The issue with and without it is confusion.

[Wren6991]

I spent some time reading the PL011 source to understand the actual reason for the requirement in the PL011 manual.

• The CDC between PCLK and UCLK domain should be safe no matter when LCR/IBRD/FBRD are written
• The baud divider counter always reloads when LCR is written, so this should always be safe, and the only symptom is a stretched baud period due to a potential partial count followed by reload.
• The rest of the LCR (the 8 actual bits in UARTLCR_H) ignores updates if failing a (UCLK-synchronous) check that neither a TX nor RX character is in progress. Most LCR_H writes are dropped whilst the UART is active. The LCR is either written or not -- no funny in-between behaviour. This seems consistent with the problems people have seen in the wild.
• The status flag for RX busy exists inside of the UART, but is not exposed through any status or debug signals.

I can't guess why Arm added that check -- maybe they were being defensive against the TX/RX state machines reaching bad states when the format changes part way.

So based on this, I think:

• The baud rate can be safely changed at any time -- the wording in the TRM is too conservative here, and it is only the bits in LCR_H itself that are problematic, even though the LCR_H write is what triggers the BRD update in the UCLK domain
• The above has one caveat which is that the first new free-running baud period may be stretched by (as long as) 1/16th of the old baud period, which may cause issues if the old baud period is long and the new is short
• There are only two ways to safely change frame format etc once the UART has already entered the enabled state:
  • Reset the UART
  • Delay for one character period after disabling the UART (like in the draft PR)

The delay is ugly, so I am tempted to leave the baud rate change unpatched (as it should work fine based on reading the implementation, and empirically does work), add a new method to initialise the UART without enabling, and add a new requirement to the format change functions that they can only be called before the UART is enabled.

I can see why it's attractive to just reset the hardware with each configuration change. PL011 doesn't make this easy.