Overall, this peripheral comes across as substantially under-opinionated to me. Essentially all the logic and behavior is in the PHY, so this really just specifies a schema for connecting a UART peripheral to the CSR bus, rather than being a peripheral in its own right. But it's limiting because the PHY is going to have to be closely coupled to the peripheral due to the shared interface prescribing the features. I don't think there's anything that could be added without changing this, except for DMA.

I would be strongly in favor of dropping the possibilities of multiple PHYs and just dragging in (or duplicating) amaranth-stdio for the implementation.

Some more specific comments:

• What does "receiver/transmitter held in reset" really mean considering the peripheral does not own the PHYs? Presumably the enable signal should be passed to them, or to the user to hook up an EnableInserter?
• How does the prescaler get wired up? Again, the peripheral does not own the PHYs?
• The baud error presumably depends on source clock frequency?
• Should the read register bit be called valid to match streams?
• The baudrate computation is described with two different syntaxes.
• The peripheral has to have at least one character of buffer for the amaranth-stdio PHY to work, why not allow it to be expanded?
• The amaranth-stdio PHY is not a valid stream transmitter so connecting it here feels slightly dirty.

I would be strongly in favor of dropping the possibilities of multiple PHYs and just dragging in (or duplicating) amaranth-stdio for the implementation.

The RFC is explicitly designed to be usable with anything that comes with a pair of streams. Furthermore, nothing in Amaranth SoC is supposed to duplicate or even closely couple to Amaranth stdio. I don't expect that to change.

The advantage of being able to couple to anything that's a pair of streams is that the peripheral becomes truly a "basic universal asynchronous receiver/transmitter" peripheral rather than an RS-232 peripheral that most "UART"s are. I consider this a highly desirable property in its own right. It is not intended to replicate the 16550 interface in the slightest.

• The baud error presumably depends on source clock frequency?

I came up with the packing scheme for 3 scale bits and 13 mantissa bits, and it has less than 1% error for all bauds from 9600 to 3M, and all clocks from 32k to 200M, except for these:

Fclk=96_000_000 req=9_600 act=11_718 error=18.07%Fclk=120_000_000 req=9_600 act=14_648 error=34.46%Fclk=150_000_000 req=9_600 act=18_310 error=47.57%Fclk=200_000_000 req=9_600 act=24_414 error=60.68%Fclk=200_000_000 req=19_200 act=24_414 error=21.36%safe=378 unsafe=5 unmet=0

Unless you're really into 9600 baud this is good enough. You can alsoplay with the script.

• Should the read register bit be called valid to match streams?

No, that is of no concern to the firmware author.

• The amaranth-stdio PHY is not a valid stream transmitter so connecting it here feels slightly dirty.

The entire current amaranth-stdio PHY will be removed and replaced with a new one based on an actual methodology rather than some scribbles I did five years ago.

I would be strongly in favor of dropping the possibilities of multiple PHYs and just dragging in (or duplicating) amaranth-stdio for the implementation.

The RFC is explicitly designed to be usable with anything that comes with a pair of streams. Furthermore, nothing in Amaranth SoC is supposed to duplicate or even closely couple to Amaranth stdio. I don't expect that to change.

I personally agree with not duplicating _stdio things in _soc. I do find though that current implementation needs quite some boilerplate code for each UART instantiation. I think the divisor and the pins are what will be specific for each instance and the rest will be boilerplate. I think it would be good to have convenience function that has the boilerplate code included.

I think it would be good to have convenience function that has the boilerplate code included.

Broadly speaking I agree but we are still at the stage where we are finding out what are the conventions and methodologies for building SoC peripherals. So I think such a function can be added at a later stage, when we have gained more understanding of what it means to build a SoC in Amaranth.

(This is also how I approach the core language, so my position should not be surprising.)

Changed to draft status, which will be removed once the UART in amaranth-stdio isrewritten from scratch updated to reach quality standards of upstream Amaranth.

This RFC was discussed during the2024-03-22 SoC meeting:

• (@zyp,@whitequark) instead ofControl andDivisor registers, provideConfig andPhyConfig registers whose shapes are user-provided and used in the PHY signatures.Config would only affect the SoC (i.e. memory-mapped) side/domain.
• (@whitequark) the goal of this UART can be described as an acronym for "Universally supports (within reason) any Asynchronous Receiver and/or Transmitter".

Updated to use aPhyConfig register (as suggested above) and shorten some API names.

The layout of theConfig register is deliberately left unparameterized:

• the peripheral relies on the existence of anenable field to drive therst port of the PHY interface.
• downstream peripheral implementations can populate the remaining 7-bitResR0W0 field while remaining register-compatible with this design iteration.

I think it's worth adding a few words on the intended CDC architecture as it's a common pain point with things like UARTs.

It's desirable to support a separate baud-reference clock from the bus clock, so that you don't have to reconfigure the UART when dynamically scaling the bus frequency. At the same time, you don't want to have to expose details of the CDC strategy to the user (like PL011 having the weird LCR write latching behaviour, and forbidding changing some settings whilst the UART is running). Ideally you want to avoid unstructured CDC between CSRs and internals/PHY.

Conversely you don't want to just have a bus CDC going into a single-domain UART implementation running from the reference clock, because this causes severe bus stalls when the reference domain is much slower than the bus clock.

One way to get around this is to split your bus into two segments:

• A synchronous bus interface providing access to async data FIFOs and their bus-side status flags
• An asynchronous bus interface (i.e. a bus CDC) into the reference domain to access all other CSRs, which can then connect synchronously to internals and PHY

This style of CDC architecture avoids bus stalls for the common case of FIFO + status access. You can implement it with multiple bus subordinates, or with a single subordinate and an internal bus splitter. You can also generate DMA requests from the bus-side FIFO status, which avoids having to CDC the DMA request.

I'm just trying to provoke discussion as this is a tricky topic, and this design's support for multiple PHYs is quite ambitious.

I'm on vacation until January, but I'd be interested to brainstorm this approach afterwards.