On 9/1/21 6:38 PM, Peter Delevoryas wrote: > >> On Sep 1, 2021, at 9:19 AM, Cédric Le Goater <c...@kaod.org> wrote: >> >> On 9/1/21 5:36 PM, p...@fb.com wrote: >>> From: Peter Delevoryas <p...@fb.com> >>> >>> When you run QEMU with an Aspeed machine and a single serial device >>> using stdio like this: >>> >>> qemu -machine ast2600-evb -drive ... -serial stdio >>> >>> The guest OS can read and write to the UART5 registers at 0x1E784000 and >>> it will receive from stdin and write to stdout. The Aspeed SoC's have a >>> lot more UART's though (AST2500 has 5, AST2600 has 13) and depending on >>> the board design, may be using any of them as the serial console. (See >>> "stdout-path" in a DTS to check which one is chosen). >>> >>> Most boards, including all of those currently defined in >>> hw/arm/aspeed.c, just use UART5, but some use UART1. This change adds >>> some flexibility for different boards without requiring users to change >>> their command-line invocation of QEMU. >>> >>> I tested this doesn't break existing code by booting an AST2500 OpenBMC >>> image and an AST2600 OpenBMC image, each using UART5 as the console. >>> >>> Then I tested switching the default to UART1 and booting an AST2600 >>> OpenBMC image that uses UART1, and that worked too. >>> >>> Signed-off-by: Peter Delevoryas <p...@fb.com> >> >> One comment, any how >> >> Reviewed-by: Cédric Le Goater <c...@kaod.org> >> >>> --- >>> hw/arm/aspeed.c | 3 +++ >>> hw/arm/aspeed_ast2600.c | 8 ++++---- >>> hw/arm/aspeed_soc.c | 9 ++++++--- >>> include/hw/arm/aspeed.h | 1 + >>> include/hw/arm/aspeed_soc.h | 1 + >>> 5 files changed, 15 insertions(+), 7 deletions(-) >>> >>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c >>> index 9d43e26c51..a81e90c539 100644 >>> --- a/hw/arm/aspeed.c >>> +++ b/hw/arm/aspeed.c >>> @@ -350,6 +350,8 @@ static void aspeed_machine_init(MachineState *machine) >>> object_property_set_int(OBJECT(&bmc->soc), "hw-prot-key", >>> ASPEED_SCU_PROT_KEY, &error_abort); >>> } >>> + qdev_prop_set_uint32(DEVICE(&bmc->soc), "uart-default", >>> + amc->uart_default); >>> qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort); >>> >>> memory_region_add_subregion(get_system_memory(), >>> @@ -804,6 +806,7 @@ static void aspeed_machine_class_init(ObjectClass *oc, >>> void *data) >>> mc->no_parallel = 1; >>> mc->default_ram_id = "ram"; >>> amc->macs_mask = ASPEED_MAC0_ON; >>> + amc->uart_default = ASPEED_DEV_UART5; >>> >>> aspeed_machine_class_props_init(oc); >>> } >>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c >>> index e3013128c6..b07fcf10a0 100644 >>> --- a/hw/arm/aspeed_ast2600.c >>> +++ b/hw/arm/aspeed_ast2600.c >>> @@ -322,10 +322,10 @@ static void aspeed_soc_ast2600_realize(DeviceState >>> *dev, Error **errp) >>> sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq); >>> } >>> >>> - /* UART - attach an 8250 to the IO space as our UART5 */ >>> - serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2, >>> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5), >>> - 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN); >>> + /* UART - attach an 8250 to the IO space as our UART */ >>> + 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); >>> >>> /* I2C */ >>> object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr), >>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c >>> index 3ad6c56fa9..09c0f83710 100644 >>> --- a/hw/arm/aspeed_soc.c >>> +++ b/hw/arm/aspeed_soc.c >>> @@ -287,9 +287,9 @@ static void aspeed_soc_realize(DeviceState *dev, Error >>> **errp) >>> sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq); >>> } >>> >>> - /* UART - attach an 8250 to the IO space as our UART5 */ >>> - serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2, >>> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5), 38400, >>> + /* UART - attach an 8250 to the IO space as our UART */ >>> + 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); >>> >>> /* I2C */ >>> @@ -439,6 +439,8 @@ static void aspeed_soc_realize(DeviceState *dev, Error >>> **errp) >>> static Property aspeed_soc_properties[] = { >>> DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION, >>> MemoryRegion *), >>> + DEFINE_PROP_UINT32("uart-default", AspeedSoCState, uart_default, >>> + ASPEED_DEV_UART5), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> >>> @@ -449,6 +451,7 @@ static void aspeed_soc_class_init(ObjectClass *oc, void >>> *data) >>> dc->realize = aspeed_soc_realize; >>> /* Reason: Uses serial_hds and nd_table in realize() directly */ >>> dc->user_creatable = false; >>> + >> >> Unneeded change, >> >> Thanks, >> >> C. > > Dang it, yeah I noticed that just after I sent the patch, my bad. I’ll remove > it, I’m just away from my computer, so probably won’t get it in time for you > guys to review over in your timeline.
Np. I fixed it inline. We are all ready for the Fuji machine now. I will send a PR after that. Thanks, C. > > Sent from my iPhone > >> >>> device_class_set_props(dc, aspeed_soc_properties); >>> } >>> >>> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h >>> index c9747b15fc..cbeacb214c 100644 >>> --- a/include/hw/arm/aspeed.h >>> +++ b/include/hw/arm/aspeed.h >>> @@ -38,6 +38,7 @@ struct AspeedMachineClass { >>> uint32_t num_cs; >>> uint32_t macs_mask; >>> void (*i2c_init)(AspeedMachineState *bmc); >>> + uint32_t uart_default; >>> }; >>> >>> >>> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h >>> index d9161d26d6..87d76c9259 100644 >>> --- a/include/hw/arm/aspeed_soc.h >>> +++ b/include/hw/arm/aspeed_soc.h >>> @@ -65,6 +65,7 @@ struct AspeedSoCState { >>> AspeedSDHCIState sdhci; >>> AspeedSDHCIState emmc; >>> AspeedLPCState lpc; >>> + uint32_t uart_default; >>> }; >>> >>> #define TYPE_ASPEED_SOC "aspeed-soc" >>> >>