Re: [PATCH] hw: aspeed_soc: Initialize all UART's with serial devices

[Peter Delevoryas]

> On May 12, 2022, at 5:37 PM, Peter Delevoryas <pdel@fb.com> wrote:
> 
> Usually, QEMU users just provide one serial device on the command line,
> either through "-nographic" or "-serial stdio -display none", or just using
> VNC and popping up a window. We try to match what the user expects, which is
> to connect the first (and usually only) serial device to the UART a board is
> using as serial0.
> 
> Most Aspeed machines in hw/arm/aspeed.c use UART5 for serial0 in their
> device tree, so we connect UART5 to the first serial device. Some machines
> use UART1 though, or UART3, so the uart_default property lets us specify
> that in a board definition.
> 
> In order to specify a nonstandard serial0 UART, a user basically *must* add
> a new board definition in hw/arm/aspeed.c. There's no way to do this without
> recompiling QEMU, besides constructing the machine completely from scratch
> on the command line.
> 
> To provide more flexibility, we can also support the user specifying more
> serial devices, and connect them to the UART memory regions if possible.
> Even if a user doesn't specify any extra serial devices, it's useful to
> initialize these memory regions as UART's, so that they respond to the guest
> OS more naturally. At the moment, they will just always return zero's for
> everything, and some UART registers have a default non-zero state.
> 
> With this change, if a new OpenBMC image uses UART3 or some other
> nonstandard UART for serial0, you can still use it with the EVB without
> recompiling QEMU, even though uart-default=UART5 for the EVB.
> 
> For example, Facebook's Wedge100 BMC uses UART3: you can fetch an image from
> Github[1] and get the serial console output even while running the palmetto
> machine type, because we explicitly specify that we want UART3 to be
> connected to stdio.
> 
>    qemu-system-arm -machine palmetto-bmc \
>        -drive file=wedge100.mtd,format=raw,if=mtd \
>        -serial null -serial null -serial null -serial stdio -display none
> 
> This is kind of complicated, of course: it might be more natural to get rid
> of the uart_default attribute completely, and initialize UART's
> sequentially. But, keeping backward compatibility and the way most users
> know how to use QEMU in mind, this seems to make the most sense.

Realized right after sending this I need to do the same thing for the AST2600 
and 1030. I’ll submit a v2 with those changes. Sorry for the noise.

> 
> [1] 
> https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd
> 
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
> hw/arm/aspeed_soc.c | 30 ++++++++++++++++++++++++++++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 58714cb2a0..5e8fea2577 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -48,6 +48,9 @@ static const hwaddr aspeed_soc_ast2400_memmap[] = {
>     [ASPEED_DEV_ETH1]   = 0x1E660000,
>     [ASPEED_DEV_ETH2]   = 0x1E680000,
>     [ASPEED_DEV_UART1]  = 0x1E783000,
> +    [ASPEED_DEV_UART2]  = 0x1E78D000,
> +    [ASPEED_DEV_UART3]  = 0x1E78E000,
> +    [ASPEED_DEV_UART4]  = 0x1E78F000,
>     [ASPEED_DEV_UART5]  = 0x1E784000,
>     [ASPEED_DEV_VUART]  = 0x1E787000,
>     [ASPEED_DEV_SDRAM]  = 0x40000000,
> @@ -80,6 +83,9 @@ static const hwaddr aspeed_soc_ast2500_memmap[] = {
>     [ASPEED_DEV_ETH1]   = 0x1E660000,
>     [ASPEED_DEV_ETH2]   = 0x1E680000,
>     [ASPEED_DEV_UART1]  = 0x1E783000,
> +    [ASPEED_DEV_UART2]  = 0x1E78D000,
> +    [ASPEED_DEV_UART3]  = 0x1E78E000,
> +    [ASPEED_DEV_UART4]  = 0x1E78F000,
>     [ASPEED_DEV_UART5]  = 0x1E784000,
>     [ASPEED_DEV_VUART]  = 0x1E787000,
>     [ASPEED_DEV_SDRAM]  = 0x80000000,
> @@ -222,7 +228,7 @@ static void aspeed_soc_init(Object *obj)
> 
> static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> {
> -    int i;
> +    int i, uart;
>     AspeedSoCState *s = ASPEED_SOC(dev);
>     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
>     Error *err = NULL;
> @@ -297,10 +303,30 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
> **errp)
>     sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
>                        aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
> 
> -    /* UART - attach an 8250 to the IO space as our UART */
> +    /*
> +     * UART - Attach the first serial device to the machine's default UART
> +     * memory region, usually corresponding to the serial0 device in the 
> device
> +     * tree.
> +     */
>     serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>                    aspeed_soc_get_irq(s, s->uart_default), 38400,
>                    serial_hd(0), DEVICE_LITTLE_ENDIAN);
> +    /*
> +     * UART - Then, initialize the remaining UART memory regions with 
> whatever
> +     * other serial devices are present. If a serial device isn't present, 
> then
> +     * the memory region still gets initialized as a UART, it just won't 
> respond
> +     * to the guest OS.
> +     */
> +    for (i = 1, uart = ASPEED_DEV_UART1; i < 5; i++, uart++) {
> +        if (uart == s->uart_default) {
> +            uart++;
> +        }
> +        assert(uart <= ASPEED_DEV_UART5);
> +
> +        serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
> +                       aspeed_soc_get_irq(s, s->uart_default), 38400,
> +                       serial_hd(i), DEVICE_LITTLE_ENDIAN);
> +    }
> 
>     /* I2C */
>     object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
> -- 
> 2.30.2
>