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

Peter Delevoryas posted 1 patch 3 days, 11 hours ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220513003720.2831997-1-pdel@fb.com
Maintainers: "Cédric Le Goater" <clg@kaod.org>, Peter Maydell <peter.maydell@linaro.org>, Andrew Jeffery <andrew@aj.id.au>, Joel Stanley <joel@jms.id.au>
hw/arm/aspeed_soc.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
[PATCH] hw: aspeed_soc: Initialize all UART's with serial devices
Posted by Peter Delevoryas 3 days, 11 hours ago
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.

[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
Re: [PATCH] hw: aspeed_soc: Initialize all UART's with serial devices
Posted by Peter Delevoryas 3 days, 10 hours ago

> 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
>