Re: [PATCH] hw/arm/virt: support both pl011 and 16550 uart

[Philippe Mathieu-Daudé]

On 23/3/23 00:27, Patrick Venture wrote:
> From: Shu-Chun Weng <scw@google.com>
>
> Select uart for virt machine from pl011 and ns16550a with
> -M virt,uart={pl011|ns16550a}.

This commit description is missing the "why" we want this change.

In which case the pl011 isn't enough or we don't want it?

> Signed-off-by: Shu-Chun Weng <scw@google.com>
> Signed-off-by: Patrick Venture <venture@google.com>
> ---
>   hw/arm/virt.c         | 85 ++++++++++++++++++++++++++++++++++++++++++-
>   include/hw/arm/virt.h |  6 +++
>   2 files changed, 89 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ac626b3bef..84b335a5d7 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -80,6 +80,7 @@
>   #include "hw/virtio/virtio-iommu.h"
>   #include "hw/char/pl011.h"
>   #include "qemu/guest-random.h"
> +#include "hw/char/serial.h"
>   
>   #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>       static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> @@ -847,8 +848,37 @@ static void create_gic(VirtMachineState *vms, MemoryRegion 
> *mem)
>       }
>   }
>   
> -static void create_uart(const VirtMachineState *vms, int uart,
> -                        MemoryRegion *mem, Chardev *chr)
> +static void create_uart_ns16550a(const VirtMachineState *vms, int uart,
> +                                 MemoryRegion *mem, Chardev *chr)
> +{
> +    char *nodename;
> +    hwaddr base = vms->memmap[uart].base;
> +    hwaddr size = vms->memmap[uart].size;
> +    int irq = vms->irqmap[uart];
> +    const char compat[] = "ns16550a";
> +
> +    serial_mm_init(get_system_memory(), base, 0,
> +                   qdev_get_gpio_in(vms->gic, irq), 19200, serial_hd(0),
> +                   DEVICE_LITTLE_ENDIAN);
> +
> +    nodename = g_strdup_printf("/serial@%" PRIx64, base);
> +
> +    MachineState *ms = MACHINE(vms);
> +
> +    qemu_fdt_add_subnode(ms->fdt, nodename);
> +    qemu_fdt_setprop(ms->fdt, nodename, "compatible", compat, sizeof(compat));
> +    qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg", 2, base, 2, size);
> +    qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "clock-frequency",
> +                                 1, 0x825f0);
> +    qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",
> +                           GIC_FDT_IRQ_TYPE_SPI, irq,
> +                           GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> +
> +    g_free(nodename);
> +}
> +
> +static void create_uart_pl011(const VirtMachineState *vms, int uart,
> +                              MemoryRegion *mem, Chardev *chr)
>   {
>       char *nodename;
>       hwaddr base = vms->memmap[uart].base;
> @@ -895,6 +925,16 @@ static void create_uart(const VirtMachineState *vms, int 
> uart,
>       g_free(nodename);
>   }
>   
> +static void create_uart(const VirtMachineState *vms, int uart,
> +                        MemoryRegion *mem, Chardev *chr)
> +{
> +    if (vms->uart == UART_NS16550A) {
> +        create_uart_ns16550a(vms, uart, mem, chr);
> +    } else {
> +        create_uart_pl011(vms, uart, mem, chr);
> +    }
> +}
> +
>   static void create_rtc(const VirtMachineState *vms)
>   {
>       char *nodename;
> @@ -2601,6 +2641,39 @@ static void virt_set_gic_version(Object *obj, const char 
> *value, Error **errp)
>       }
>   }
>   
> +static char *virt_get_uart_type(Object *obj, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +    const char *val = NULL;
> +
> +    switch (vms->uart) {
> +    case UART_PL011:
> +        val = "pl011";
> +        break;
> +    case UART_NS16550A:
> +        val = "ns16550a";
> +        break;
> +    default:
> +        error_setg(errp, "Invalid uart value");
> +    }
> +
> +    return g_strdup(val);
> +}
> +
> +static void virt_set_uart_type(Object *obj, const char *value, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    if (!strcmp(value, "pl011")) {
> +        vms->uart = UART_PL011;
> +    } else if (!strcmp(value, "ns16550a")) {
> +        vms->uart = UART_NS16550A;
> +    } else {
> +        error_setg(errp, "Invalid uart type");
> +        error_append_hint(errp, "Valid values are pl011, and ns16550a.\n");
> +    }
> +}
> +
>   static char *virt_get_iommu(Object *obj, Error **errp)
>   {
>       VirtMachineState *vms = VIRT_MACHINE(obj);
> @@ -3172,6 +3245,14 @@ static void virt_instance_init(Object *obj)
>       vms->highmem_compact = !vmc->no_highmem_compact;
>       vms->gic_version = VIRT_GIC_VERSION_NOSEL;
>   
> +    /* Default uart type is pl011 */
> +    vms->uart = UART_PL011;
> +    object_property_add_str(obj, "uart", virt_get_uart_type,
> +                            virt_set_uart_type);
> +    object_property_set_description(obj, "uart",
> +                                    "Set uart type. "
> +                                    "Valid values are pl011 and ns16550a");
> +
>       vms->highmem_ecam = !vmc->no_highmem_ecam;
>       vms->highmem_mmio = true;
>       vms->highmem_redists = true;
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index e1ddbea96b..04539f347d 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -122,6 +122,11 @@ typedef enum VirtGICType {
>   #define VIRT_GIC_VERSION_3_MASK BIT(VIRT_GIC_VERSION_3)
>   #define VIRT_GIC_VERSION_4_MASK BIT(VIRT_GIC_VERSION_4)
>   
> +typedef enum UartType {
> +    UART_PL011,
> +    UART_NS16550A,
> +} UartType;
> +
>   struct VirtMachineClass {
>       MachineClass parent;
>       bool disallow_affinity_adjustment;
> @@ -183,6 +188,7 @@ struct VirtMachineState {
>       PCIBus *bus;
>       char *oem_id;
>       char *oem_table_id;
> +    UartType uart;
>   };
>   
>   #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)