| Message ID | 20230322232704.206683-1-venture@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/virt: support both pl011 and 16550 uart | expand |
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)
On Wed, 22 Mar 2023 at 23:27, Patrick Venture <venture@google.com> wrote: > > From: Shu-Chun Weng <scw@google.com> > > Select uart for virt machine from pl011 and ns16550a with > -M virt,uart={pl011|ns16550a}. Firm "no". The PL011 is a perfectly good UART widely supported by guest OSes, and there is no reason why we should want to also provide a 16550. thanks -- PMM
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)