diff mbox series

[v2] aspeed: Refactor UART init for multi-SoC machines

Message ID 20220701200234.68289-1-me@pjd.dev
State New
Headers show
Series [v2] aspeed: Refactor UART init for multi-SoC machines | expand

Commit Message

Peter Delevoryas July 1, 2022, 8:02 p.m. UTC 
This change moves the code that connects the SoC UART's to serial_hd's
to the machine.

It makes each UART a proper child member of the SoC, and then allows the
machine to selectively initialize the chardev for each UART with a
serial_hd.

This should preserve backwards compatibility, but also allow multi-SoC
boards to completely change the wiring of serial devices from the
command line to specific SoC UART's.

This also removes the uart-default property from the SoC, since the SoC
doesn't need to know what UART is the "default" on the machine anymore.

I tested this using the images and commands from the previous
refactoring, and another test image for the ast1030:

    wget https://github.com/facebook/openbmc/releases/download/v2021.49.0/fuji.mtd
    wget https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd
    wget https://github.com/peterdelevoryas/OpenBIC/releases/download/oby35-cl-2022.13.01/Y35BCL.elf

Fuji uses UART1:

    qemu-system-arm -machine fuji-bmc \
        -drive file=fuji.mtd,format=raw,if=mtd \
        -nographic

ast2600-evb uses uart-default=UART5:

    qemu-system-arm -machine ast2600-evb \
        -drive file=fuji.mtd,format=raw,if=mtd \
        -serial null -serial mon:stdio -display none

Wedge100 uses UART3:

    qemu-system-arm -machine palmetto-bmc \
        -drive file=wedge100.mtd,format=raw,if=mtd \
        -serial null -serial null -serial null \
        -serial mon:stdio -display none

AST1030 EVB uses UART5:

    qemu-system-arm -machine ast1030-evb \
        -kernel Y35BCL.elf -nographic

Fixes: 6827ff20b2975 ("hw: aspeed: Init all UART's with serial devices")
Signed-off-by: Peter Delevoryas <me@pjd.dev>
---
 hw/arm/aspeed.c             | 23 +++++++++++++++----
 hw/arm/aspeed_ast10x0.c     |  4 ++++
 hw/arm/aspeed_ast2600.c     |  4 ++++
 hw/arm/aspeed_soc.c         | 44 ++++++++++++++++++++++++-------------
 include/hw/arm/aspeed_soc.h |  5 ++++-
 5 files changed, 60 insertions(+), 20 deletions(-)

Comments

Cédric Le Goater July 2, 2022, 5:57 a.m. UTC | #1 
On 7/1/22 22:02, Peter Delevoryas wrote:
> This change moves the code that connects the SoC UART's to serial_hd's
> to the machine.
> 
> It makes each UART a proper child member of the SoC, and then allows the
> machine to selectively initialize the chardev for each UART with a
> serial_hd.
> 
> This should preserve backwards compatibility, but also allow multi-SoC
> boards to completely change the wiring of serial devices from the
> command line to specific SoC UART's.
> 
> This also removes the uart-default property from the SoC, since the SoC
> doesn't need to know what UART is the "default" on the machine anymore.
> 
> I tested this using the images and commands from the previous
> refactoring, and another test image for the ast1030:
> 
>      wget https://github.com/facebook/openbmc/releases/download/v2021.49.0/fuji.mtd
>      wget https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd
>      wget https://github.com/peterdelevoryas/OpenBIC/releases/download/oby35-cl-2022.13.01/Y35BCL.elf
> 
> Fuji uses UART1:
> 
>      qemu-system-arm -machine fuji-bmc \
>          -drive file=fuji.mtd,format=raw,if=mtd \
>          -nographic
> 
> ast2600-evb uses uart-default=UART5:
> 
>      qemu-system-arm -machine ast2600-evb \
>          -drive file=fuji.mtd,format=raw,if=mtd \
>          -serial null -serial mon:stdio -display none
> 
> Wedge100 uses UART3:
> 
>      qemu-system-arm -machine palmetto-bmc \
>          -drive file=wedge100.mtd,format=raw,if=mtd \
>          -serial null -serial null -serial null \
>          -serial mon:stdio -display none
> 
> AST1030 EVB uses UART5:
> 
>      qemu-system-arm -machine ast1030-evb \
>          -kernel Y35BCL.elf -nographic

Looks good. A few comments on the APIs.

> 
> Fixes: 6827ff20b2975 ("hw: aspeed: Init all UART's with serial devices")
> Signed-off-by: Peter Delevoryas <me@pjd.dev>
> ---
>   hw/arm/aspeed.c             | 23 +++++++++++++++----
>   hw/arm/aspeed_ast10x0.c     |  4 ++++
>   hw/arm/aspeed_ast2600.c     |  4 ++++
>   hw/arm/aspeed_soc.c         | 44 ++++++++++++++++++++++++-------------
>   include/hw/arm/aspeed_soc.h |  5 ++++-
>   5 files changed, 60 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 6fe9b13548..fdca0abd95 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -26,6 +26,7 @@
>   #include "qemu/error-report.h"
>   #include "qemu/units.h"
>   #include "hw/qdev-clock.h"
> +#include "sysemu/sysemu.h"
>   
>   static struct arm_boot_info aspeed_board_binfo = {
>       .board_id = -1, /* device-tree-only board */
> @@ -301,6 +302,22 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo)
>                                  &error_fatal);
>   }
>   
> +static void connect_serial_hds_to_uarts(AspeedMachineState *bmc)

something like :

   void aspeed_soc_uart_connect(AspeedSoCState *s, int uart_default)

which could be exported from aspeed_soc.c

> +{
> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);

and you wouldn't need the machine.

> +    AspeedSoCState *s = &bmc->soc;
> +
> +    aspeed_soc_uart_set_chr(s, amc->uart_default, serial_hd(0));
> +    for (int i = 1, uart = ASPEED_DEV_UART1;
> +         serial_hd(i) && uart <= ASPEED_DEV_UART13; i++, uart++) {

We should test for :

   ASPEED_SOC_GET_CLASS(s)->uarts_num

I am not sure we want to stop the loop if serial_hd(i) is NULL ?

> +
> +        if (uart == amc->uart_default) {
> +            continue;
> +        }
> +        aspeed_soc_uart_set_chr(s, uart, serial_hd(i));
> +    }
> +}>   static void aspeed_machine_init(MachineState *machine)
>   {
>       AspeedMachineState *bmc = ASPEED_MACHINE(machine);
> @@ -346,8 +363,7 @@ 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);
> +    connect_serial_hds_to_uarts(bmc);
>       qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
>   
>       aspeed_board_init_flashes(&bmc->soc.fmc,
> @@ -1383,8 +1399,7 @@ static void aspeed_minibmc_machine_init(MachineState *machine)
>   
>       object_property_set_link(OBJECT(&bmc->soc), "memory",
>                                OBJECT(get_system_memory()), &error_abort);
> -    qdev_prop_set_uint32(DEVICE(&bmc->soc), "uart-default",
> -                         amc->uart_default);
> +    connect_serial_hds_to_uarts(bmc);
>       qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
>   
>       aspeed_board_init_flashes(&bmc->soc.fmc,
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index 33ef331771..a221f5d6fe 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -144,6 +144,10 @@ static void aspeed_soc_ast1030_init(Object *obj)
>           object_initialize_child(obj, "wdt[*]", &s->wdt[i], typename);
>       }
>   
> +    for (i = 0; i < sc->uarts_num; i++) {
> +        object_initialize_child(obj, "uart[*]", &s->uart[i], TYPE_SERIAL_MM);
> +    }
> +
>       snprintf(typename, sizeof(typename), "aspeed.gpio-%s", socname);
>       object_initialize_child(obj, "gpio", &s->gpio, typename);
>   
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 3f0611ac11..c4ad26a046 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -214,6 +214,10 @@ static void aspeed_soc_ast2600_init(Object *obj)
>           object_initialize_child(obj, "mii[*]", &s->mii[i], TYPE_ASPEED_MII);
>       }
>   
> +    for (i = 0; i < sc->uarts_num; i++) {
> +        object_initialize_child(obj, "uart[*]", &s->uart[i], TYPE_SERIAL_MM);
> +    }
> +
>       snprintf(typename, sizeof(typename), TYPE_ASPEED_XDMA "-%s", socname);
>       object_initialize_child(obj, "xdma", &s->xdma, typename);
>   
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 0f675e7fcd..2ac18cbf27 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -208,6 +208,10 @@ static void aspeed_soc_init(Object *obj)
>                                   TYPE_FTGMAC100);
>       }
>   
> +    for (i = 0; i < sc->uarts_num; i++) {
> +        object_initialize_child(obj, "uart[*]", &s->uart[i], TYPE_SERIAL_MM);
> +    }
> +
>       snprintf(typename, sizeof(typename), TYPE_ASPEED_XDMA "-%s", socname);
>       object_initialize_child(obj, "xdma", &s->xdma, typename);
>   
> @@ -481,8 +485,6 @@ static Property aspeed_soc_properties[] = {
>                        MemoryRegion *),
>       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(),
>   };
>   
> @@ -575,22 +577,34 @@ qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int dev)
>   void aspeed_soc_uart_init(AspeedSoCState *s)

We can handle errors now. So :

   bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp)

>   {
>       AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> -    int i, uart;
> -
> -    /* Attach an 8250 to the IO space as our UART */
> -    serial_mm_init(s->memory, sc->memmap[s->uart_default], 2,
> -                   aspeed_soc_get_irq(s, s->uart_default), 38400,
> -                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
> -    for (i = 1, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
> -        if (uart == s->uart_default) {
> -            uart++;
> -        }
> -        serial_mm_init(s->memory, sc->memmap[uart], 2,
> -                       aspeed_soc_get_irq(s, uart), 38400,
> -                       serial_hd(i), DEVICE_LITTLE_ENDIAN);
> +    SerialMM *smm;
> +    MemoryRegion *mr;
> +
> +    for (int i = 0, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
> +        smm = &s->uart[i];
> +
> +        /* Chardev property is set by the machine. */
> +        qdev_prop_set_uint8(DEVICE(smm), "regshift", 2);
> +        qdev_prop_set_uint32(DEVICE(smm), "baudbase", 38400);
> +        qdev_set_legacy_instance_id(DEVICE(smm), sc->memmap[uart], 2);
> +        qdev_prop_set_uint8(DEVICE(smm), "endianness", DEVICE_LITTLE_ENDIAN);
> +        sysbus_realize(SYS_BUS_DEVICE(smm), &error_fatal);

use errp instead and return false in case of failure.

> +
> +        sysbus_connect_irq(SYS_BUS_DEVICE(smm), 0, aspeed_soc_get_irq(s, uart));
> +        mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(smm), 0);
> +        memory_region_add_subregion(s->memory, sc->memmap[uart], mr);

You introduced aspeed_mmio_map() :)

>       }
>   }
>   
> +void aspeed_soc_uart_set_chr(AspeedSoCState *s, int dev, Chardev *chr)

you could merge this routine in aspeed_soc_uart_connect() I think.

Thanks,

C.


> +{
> +    AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> +    int i = dev - ASPEED_DEV_UART1;
> +
> +    g_assert(0 <= i && i < ARRAY_SIZE(s->uart) && i < sc->uarts_num);
> +    qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
> +}
> +
>   /*
>    * SDMC should be realized first to get correct RAM size and max size
>    * values
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index e65926a667..60ee0a84db 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -36,12 +36,14 @@
>   #include "hw/misc/aspeed_lpc.h"
>   #include "hw/misc/unimp.h"
>   #include "hw/misc/aspeed_peci.h"
> +#include "hw/char/serial.h"
>   
>   #define ASPEED_SPIS_NUM  2
>   #define ASPEED_EHCIS_NUM 2
>   #define ASPEED_WDTS_NUM  4
>   #define ASPEED_CPUS_NUM  2
>   #define ASPEED_MACS_NUM  4
> +#define ASPEED_UARTS_NUM 13
>   
>   struct AspeedSoCState {
>       /*< private >*/
> @@ -79,7 +81,7 @@ struct AspeedSoCState {
>       AspeedSDHCIState emmc;
>       AspeedLPCState lpc;
>       AspeedPECIState peci;
> -    uint32_t uart_default;
> +    SerialMM uart[ASPEED_UARTS_NUM];
>       Clock *sysclk;
>       UnimplementedDeviceState iomem;
>       UnimplementedDeviceState video;
> @@ -176,6 +178,7 @@ enum {
>   
>   qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int dev);
>   void aspeed_soc_uart_init(AspeedSoCState *s);
> +void aspeed_soc_uart_set_chr(AspeedSoCState *s, int dev, Chardev *chr);
>   bool aspeed_soc_dram_init(AspeedSoCState *s, Error **errp);
>   void aspeed_mmio_map(AspeedSoCState *s, SysBusDevice *dev, int n, hwaddr addr);
>   void aspeed_mmio_map_unimplemented(AspeedSoCState *s, SysBusDevice *dev,
Peter Delevoryas July 2, 2022, 6:44 a.m. UTC | #2 
On Sat, Jul 02, 2022 at 07:57:09AM +0200, Cédric Le Goater wrote:
> On 7/1/22 22:02, Peter Delevoryas wrote:
> > This change moves the code that connects the SoC UART's to serial_hd's
> > to the machine.
> > 
> > It makes each UART a proper child member of the SoC, and then allows the
> > machine to selectively initialize the chardev for each UART with a
> > serial_hd.
> > 
> > This should preserve backwards compatibility, but also allow multi-SoC
> > boards to completely change the wiring of serial devices from the
> > command line to specific SoC UART's.
> > 
> > This also removes the uart-default property from the SoC, since the SoC
> > doesn't need to know what UART is the "default" on the machine anymore.
> > 
> > I tested this using the images and commands from the previous
> > refactoring, and another test image for the ast1030:
> > 
> >      wget https://github.com/facebook/openbmc/releases/download/v2021.49.0/fuji.mtd
> >      wget https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd
> >      wget https://github.com/peterdelevoryas/OpenBIC/releases/download/oby35-cl-2022.13.01/Y35BCL.elf
> > 
> > Fuji uses UART1:
> > 
> >      qemu-system-arm -machine fuji-bmc \
> >          -drive file=fuji.mtd,format=raw,if=mtd \
> >          -nographic
> > 
> > ast2600-evb uses uart-default=UART5:
> > 
> >      qemu-system-arm -machine ast2600-evb \
> >          -drive file=fuji.mtd,format=raw,if=mtd \
> >          -serial null -serial mon:stdio -display none
> > 
> > Wedge100 uses UART3:
> > 
> >      qemu-system-arm -machine palmetto-bmc \
> >          -drive file=wedge100.mtd,format=raw,if=mtd \
> >          -serial null -serial null -serial null \
> >          -serial mon:stdio -display none
> > 
> > AST1030 EVB uses UART5:
> > 
> >      qemu-system-arm -machine ast1030-evb \
> >          -kernel Y35BCL.elf -nographic
> 
> Looks good. A few comments on the APIs.
> 
> > 
> > Fixes: 6827ff20b2975 ("hw: aspeed: Init all UART's with serial devices")
> > Signed-off-by: Peter Delevoryas <me@pjd.dev>
> > ---
> >   hw/arm/aspeed.c             | 23 +++++++++++++++----
> >   hw/arm/aspeed_ast10x0.c     |  4 ++++
> >   hw/arm/aspeed_ast2600.c     |  4 ++++
> >   hw/arm/aspeed_soc.c         | 44 ++++++++++++++++++++++++-------------
> >   include/hw/arm/aspeed_soc.h |  5 ++++-
> >   5 files changed, 60 insertions(+), 20 deletions(-)
> > 
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > index 6fe9b13548..fdca0abd95 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -26,6 +26,7 @@
> >   #include "qemu/error-report.h"
> >   #include "qemu/units.h"
> >   #include "hw/qdev-clock.h"
> > +#include "sysemu/sysemu.h"
> >   static struct arm_boot_info aspeed_board_binfo = {
> >       .board_id = -1, /* device-tree-only board */
> > @@ -301,6 +302,22 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo)
> >                                  &error_fatal);
> >   }
> > +static void connect_serial_hds_to_uarts(AspeedMachineState *bmc)
> 
> something like :
> 
>   void aspeed_soc_uart_connect(AspeedSoCState *s, int uart_default)
> 
> which could be exported from aspeed_soc.c

Actually I kinda disagree on this.

In aspeed_soc.h, I added

    aspeed_soc_uart_set_chr(AspeedSoCState *s, int dev, Chardev *chr)

This is to allow Aspeed SoC users to set the Chardev for each UART.

Then, in aspeed.c, I added connect_serial_hds_to_uarts to perform the wiring
functionality that we've expected up to this point, which is to connect all the
serial devices to one SoC.

I'm imagining that we would use aspeed_soc_uart_set_chr in the fby35
machine like this:

diff --git a/hw/arm/fby35.c b/hw/arm/fby35.c
index 0755e3f04f..85abf67b23 100644
--- a/hw/arm/fby35.c
+++ b/hw/arm/fby35.c
@@ -26,6 +26,7 @@
 #include "hw/boards.h"
 #include "hw/qdev-clock.h"
 #include "sysemu/block-backend.h"
+#include "sysemu/sysemu.h"
 #include "hw/arm/aspeed_soc.h"
 #include "hw/arm/boot.h"

@@ -99,7 +100,7 @@ static void fby35_bmc_init(Fby35State *s)
     object_property_set_link(OBJECT(&s->bmc), "dram", OBJECT(&s->bmc_dram), &error_abort);
     object_property_set_int(OBJECT(&s->bmc), "hw-strap1", 0x000000C0, &error_abort);
     object_property_set_int(OBJECT(&s->bmc), "hw-strap2", 0x00000003, &error_abort);
-    object_property_set_int(OBJECT(&s->bmc), "uart-default", ASPEED_DEV_UART5, &error_abort);
+    aspeed_soc_uart_set_chr(&s->bmc, ASPEED_DEV_UART5, serial_hd(0));
     qdev_realize(DEVICE(&s->bmc), NULL, &error_abort);

     aspeed_board_init_flashes(&s->bmc.fmc, "n25q00", 2, 0);
@@ -136,7 +137,7 @@ static void fby35_bic_init(Fby35State *s)
     object_initialize_child(OBJECT(s), "bic", &s->bic, "ast1030-a1");
     qdev_connect_clock_in(DEVICE(&s->bic), "sysclk", s->bic_sysclk);
     object_property_set_link(OBJECT(&s->bic), "memory", OBJECT(&s->bic_memory), &error_abort);
-    qdev_prop_set_uint32(DEVICE(&s->bic), "uart-default", ASPEED_DEV_UART5);
+    aspeed_soc_uart_set_chr(&s->bic, ASPEED_DEV_UART5, serial_hd(1));
     qdev_realize(DEVICE(&s->bic), NULL, &error_abort);

     aspeed_board_init_flashes(&s->bic.fmc, "sst25vf032b", 2, 2);

We could add aspeed_soc_uart_connect, but I'm struggling to see how it could
cover both single-SoC and multi-SoC boards while still preserving the feature
in single-SoC that lets us experiment with UART1/UART3 on EVB's.

> 
> > +{
> > +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
> 
> and you wouldn't need the machine.
> 
> > +    AspeedSoCState *s = &bmc->soc;
> > +
> > +    aspeed_soc_uart_set_chr(s, amc->uart_default, serial_hd(0));
> > +    for (int i = 1, uart = ASPEED_DEV_UART1;
> > +         serial_hd(i) && uart <= ASPEED_DEV_UART13; i++, uart++) {
> 
> We should test for :
> 
>   ASPEED_SOC_GET_CLASS(s)->uarts_num

Oh good call, yeah I'll add that.

> 
> I am not sure we want to stop the loop if serial_hd(i) is NULL ?

Yikes! Yeah I was not thinking clearly when I wrote this. Nice catch!

> 
> > +
> > +        if (uart == amc->uart_default) {
> > +            continue;
> > +        }
> > +        aspeed_soc_uart_set_chr(s, uart, serial_hd(i));
> > +    }
> > +}>   static void aspeed_machine_init(MachineState *machine)
> >   {
> >       AspeedMachineState *bmc = ASPEED_MACHINE(machine);
> > @@ -346,8 +363,7 @@ 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);
> > +    connect_serial_hds_to_uarts(bmc);
> >       qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
> >       aspeed_board_init_flashes(&bmc->soc.fmc,
> > @@ -1383,8 +1399,7 @@ static void aspeed_minibmc_machine_init(MachineState *machine)
> >       object_property_set_link(OBJECT(&bmc->soc), "memory",
> >                                OBJECT(get_system_memory()), &error_abort);
> > -    qdev_prop_set_uint32(DEVICE(&bmc->soc), "uart-default",
> > -                         amc->uart_default);
> > +    connect_serial_hds_to_uarts(bmc);
> >       qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
> >       aspeed_board_init_flashes(&bmc->soc.fmc,
> > diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> > index 33ef331771..a221f5d6fe 100644
> > --- a/hw/arm/aspeed_ast10x0.c
> > +++ b/hw/arm/aspeed_ast10x0.c
> > @@ -144,6 +144,10 @@ static void aspeed_soc_ast1030_init(Object *obj)
> >           object_initialize_child(obj, "wdt[*]", &s->wdt[i], typename);
> >       }
> > +    for (i = 0; i < sc->uarts_num; i++) {
> > +        object_initialize_child(obj, "uart[*]", &s->uart[i], TYPE_SERIAL_MM);
> > +    }
> > +
> >       snprintf(typename, sizeof(typename), "aspeed.gpio-%s", socname);
> >       object_initialize_child(obj, "gpio", &s->gpio, typename);
> > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> > index 3f0611ac11..c4ad26a046 100644
> > --- a/hw/arm/aspeed_ast2600.c
> > +++ b/hw/arm/aspeed_ast2600.c
> > @@ -214,6 +214,10 @@ static void aspeed_soc_ast2600_init(Object *obj)
> >           object_initialize_child(obj, "mii[*]", &s->mii[i], TYPE_ASPEED_MII);
> >       }
> > +    for (i = 0; i < sc->uarts_num; i++) {
> > +        object_initialize_child(obj, "uart[*]", &s->uart[i], TYPE_SERIAL_MM);
> > +    }
> > +
> >       snprintf(typename, sizeof(typename), TYPE_ASPEED_XDMA "-%s", socname);
> >       object_initialize_child(obj, "xdma", &s->xdma, typename);
> > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> > index 0f675e7fcd..2ac18cbf27 100644
> > --- a/hw/arm/aspeed_soc.c
> > +++ b/hw/arm/aspeed_soc.c
> > @@ -208,6 +208,10 @@ static void aspeed_soc_init(Object *obj)
> >                                   TYPE_FTGMAC100);
> >       }
> > +    for (i = 0; i < sc->uarts_num; i++) {
> > +        object_initialize_child(obj, "uart[*]", &s->uart[i], TYPE_SERIAL_MM);
> > +    }
> > +
> >       snprintf(typename, sizeof(typename), TYPE_ASPEED_XDMA "-%s", socname);
> >       object_initialize_child(obj, "xdma", &s->xdma, typename);
> > @@ -481,8 +485,6 @@ static Property aspeed_soc_properties[] = {
> >                        MemoryRegion *),
> >       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(),
> >   };
> > @@ -575,22 +577,34 @@ qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int dev)
> >   void aspeed_soc_uart_init(AspeedSoCState *s)
> 
> We can handle errors now. So :
> 
>   bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp)

Good idea, +1

> 
> >   {
> >       AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> > -    int i, uart;
> > -
> > -    /* Attach an 8250 to the IO space as our UART */
> > -    serial_mm_init(s->memory, sc->memmap[s->uart_default], 2,
> > -                   aspeed_soc_get_irq(s, s->uart_default), 38400,
> > -                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
> > -    for (i = 1, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
> > -        if (uart == s->uart_default) {
> > -            uart++;
> > -        }
> > -        serial_mm_init(s->memory, sc->memmap[uart], 2,
> > -                       aspeed_soc_get_irq(s, uart), 38400,
> > -                       serial_hd(i), DEVICE_LITTLE_ENDIAN);
> > +    SerialMM *smm;
> > +    MemoryRegion *mr;
> > +
> > +    for (int i = 0, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
> > +        smm = &s->uart[i];
> > +
> > +        /* Chardev property is set by the machine. */
> > +        qdev_prop_set_uint8(DEVICE(smm), "regshift", 2);
> > +        qdev_prop_set_uint32(DEVICE(smm), "baudbase", 38400);
> > +        qdev_set_legacy_instance_id(DEVICE(smm), sc->memmap[uart], 2);
> > +        qdev_prop_set_uint8(DEVICE(smm), "endianness", DEVICE_LITTLE_ENDIAN);
> > +        sysbus_realize(SYS_BUS_DEVICE(smm), &error_fatal);
> 
> use errp instead and return false in case of failure.

+1

> 
> > +
> > +        sysbus_connect_irq(SYS_BUS_DEVICE(smm), 0, aspeed_soc_get_irq(s, uart));
> > +        mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(smm), 0);
> > +        memory_region_add_subregion(s->memory, sc->memmap[uart], mr);
> 
> You introduced aspeed_mmio_map() :)

Hahahaha you're totally right, I was _really_ not thinking very clearly this
morning.

> 
> >       }
> >   }
> > +void aspeed_soc_uart_set_chr(AspeedSoCState *s, int dev, Chardev *chr)
> 
> you could merge this routine in aspeed_soc_uart_connect() I think.

See my comment at the start about this, I'm not sure how we could
use aspeed_soc_uart_connect with the multi-SoC board, but maybe
I'm missing something.

Thanks,
Peter

> 
> Thanks,
> 
> C.
> 
> 
> > +{
> > +    AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> > +    int i = dev - ASPEED_DEV_UART1;
> > +
> > +    g_assert(0 <= i && i < ARRAY_SIZE(s->uart) && i < sc->uarts_num);
> > +    qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
> > +}
> > +
> >   /*
> >    * SDMC should be realized first to get correct RAM size and max size
> >    * values
> > diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> > index e65926a667..60ee0a84db 100644
> > --- a/include/hw/arm/aspeed_soc.h
> > +++ b/include/hw/arm/aspeed_soc.h
> > @@ -36,12 +36,14 @@
> >   #include "hw/misc/aspeed_lpc.h"
> >   #include "hw/misc/unimp.h"
> >   #include "hw/misc/aspeed_peci.h"
> > +#include "hw/char/serial.h"
> >   #define ASPEED_SPIS_NUM  2
> >   #define ASPEED_EHCIS_NUM 2
> >   #define ASPEED_WDTS_NUM  4
> >   #define ASPEED_CPUS_NUM  2
> >   #define ASPEED_MACS_NUM  4
> > +#define ASPEED_UARTS_NUM 13
> >   struct AspeedSoCState {
> >       /*< private >*/
> > @@ -79,7 +81,7 @@ struct AspeedSoCState {
> >       AspeedSDHCIState emmc;
> >       AspeedLPCState lpc;
> >       AspeedPECIState peci;
> > -    uint32_t uart_default;
> > +    SerialMM uart[ASPEED_UARTS_NUM];
> >       Clock *sysclk;
> >       UnimplementedDeviceState iomem;
> >       UnimplementedDeviceState video;
> > @@ -176,6 +178,7 @@ enum {
> >   qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int dev);
> >   void aspeed_soc_uart_init(AspeedSoCState *s);
> > +void aspeed_soc_uart_set_chr(AspeedSoCState *s, int dev, Chardev *chr);
> >   bool aspeed_soc_dram_init(AspeedSoCState *s, Error **errp);
> >   void aspeed_mmio_map(AspeedSoCState *s, SysBusDevice *dev, int n, hwaddr addr);
> >   void aspeed_mmio_map_unimplemented(AspeedSoCState *s, SysBusDevice *dev,
>
diff mbox series

Patch

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 6fe9b13548..fdca0abd95 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -26,6 +26,7 @@ 
 #include "qemu/error-report.h"
 #include "qemu/units.h"
 #include "hw/qdev-clock.h"
+#include "sysemu/sysemu.h"
 
 static struct arm_boot_info aspeed_board_binfo = {
     .board_id = -1, /* device-tree-only board */
@@ -301,6 +302,22 @@  static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo)
                                &error_fatal);
 }
 
+static void connect_serial_hds_to_uarts(AspeedMachineState *bmc)
+{
+    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
+    AspeedSoCState *s = &bmc->soc;
+
+    aspeed_soc_uart_set_chr(s, amc->uart_default, serial_hd(0));
+    for (int i = 1, uart = ASPEED_DEV_UART1;
+         serial_hd(i) && uart <= ASPEED_DEV_UART13; i++, uart++) {
+
+        if (uart == amc->uart_default) {
+            continue;
+        }
+        aspeed_soc_uart_set_chr(s, uart, serial_hd(i));
+    }
+}
+
 static void aspeed_machine_init(MachineState *machine)
 {
     AspeedMachineState *bmc = ASPEED_MACHINE(machine);
@@ -346,8 +363,7 @@  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);
+    connect_serial_hds_to_uarts(bmc);
     qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
 
     aspeed_board_init_flashes(&bmc->soc.fmc,
@@ -1383,8 +1399,7 @@  static void aspeed_minibmc_machine_init(MachineState *machine)
 
     object_property_set_link(OBJECT(&bmc->soc), "memory",
                              OBJECT(get_system_memory()), &error_abort);
-    qdev_prop_set_uint32(DEVICE(&bmc->soc), "uart-default",
-                         amc->uart_default);
+    connect_serial_hds_to_uarts(bmc);
     qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
 
     aspeed_board_init_flashes(&bmc->soc.fmc,
diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index 33ef331771..a221f5d6fe 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -144,6 +144,10 @@  static void aspeed_soc_ast1030_init(Object *obj)
         object_initialize_child(obj, "wdt[*]", &s->wdt[i], typename);
     }
 
+    for (i = 0; i < sc->uarts_num; i++) {
+        object_initialize_child(obj, "uart[*]", &s->uart[i], TYPE_SERIAL_MM);
+    }
+
     snprintf(typename, sizeof(typename), "aspeed.gpio-%s", socname);
     object_initialize_child(obj, "gpio", &s->gpio, typename);
 
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 3f0611ac11..c4ad26a046 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -214,6 +214,10 @@  static void aspeed_soc_ast2600_init(Object *obj)
         object_initialize_child(obj, "mii[*]", &s->mii[i], TYPE_ASPEED_MII);
     }
 
+    for (i = 0; i < sc->uarts_num; i++) {
+        object_initialize_child(obj, "uart[*]", &s->uart[i], TYPE_SERIAL_MM);
+    }
+
     snprintf(typename, sizeof(typename), TYPE_ASPEED_XDMA "-%s", socname);
     object_initialize_child(obj, "xdma", &s->xdma, typename);
 
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 0f675e7fcd..2ac18cbf27 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -208,6 +208,10 @@  static void aspeed_soc_init(Object *obj)
                                 TYPE_FTGMAC100);
     }
 
+    for (i = 0; i < sc->uarts_num; i++) {
+        object_initialize_child(obj, "uart[*]", &s->uart[i], TYPE_SERIAL_MM);
+    }
+
     snprintf(typename, sizeof(typename), TYPE_ASPEED_XDMA "-%s", socname);
     object_initialize_child(obj, "xdma", &s->xdma, typename);
 
@@ -481,8 +485,6 @@  static Property aspeed_soc_properties[] = {
                      MemoryRegion *),
     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(),
 };
 
@@ -575,22 +577,34 @@  qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int dev)
 void aspeed_soc_uart_init(AspeedSoCState *s)
 {
     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
-    int i, uart;
-
-    /* Attach an 8250 to the IO space as our UART */
-    serial_mm_init(s->memory, sc->memmap[s->uart_default], 2,
-                   aspeed_soc_get_irq(s, s->uart_default), 38400,
-                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
-    for (i = 1, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
-        if (uart == s->uart_default) {
-            uart++;
-        }
-        serial_mm_init(s->memory, sc->memmap[uart], 2,
-                       aspeed_soc_get_irq(s, uart), 38400,
-                       serial_hd(i), DEVICE_LITTLE_ENDIAN);
+    SerialMM *smm;
+    MemoryRegion *mr;
+
+    for (int i = 0, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
+        smm = &s->uart[i];
+
+        /* Chardev property is set by the machine. */
+        qdev_prop_set_uint8(DEVICE(smm), "regshift", 2);
+        qdev_prop_set_uint32(DEVICE(smm), "baudbase", 38400);
+        qdev_set_legacy_instance_id(DEVICE(smm), sc->memmap[uart], 2);
+        qdev_prop_set_uint8(DEVICE(smm), "endianness", DEVICE_LITTLE_ENDIAN);
+        sysbus_realize(SYS_BUS_DEVICE(smm), &error_fatal);
+
+        sysbus_connect_irq(SYS_BUS_DEVICE(smm), 0, aspeed_soc_get_irq(s, uart));
+        mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(smm), 0);
+        memory_region_add_subregion(s->memory, sc->memmap[uart], mr);
     }
 }
 
+void aspeed_soc_uart_set_chr(AspeedSoCState *s, int dev, Chardev *chr)
+{
+    AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
+    int i = dev - ASPEED_DEV_UART1;
+
+    g_assert(0 <= i && i < ARRAY_SIZE(s->uart) && i < sc->uarts_num);
+    qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
+}
+
 /*
  * SDMC should be realized first to get correct RAM size and max size
  * values
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index e65926a667..60ee0a84db 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -36,12 +36,14 @@ 
 #include "hw/misc/aspeed_lpc.h"
 #include "hw/misc/unimp.h"
 #include "hw/misc/aspeed_peci.h"
+#include "hw/char/serial.h"
 
 #define ASPEED_SPIS_NUM  2
 #define ASPEED_EHCIS_NUM 2
 #define ASPEED_WDTS_NUM  4
 #define ASPEED_CPUS_NUM  2
 #define ASPEED_MACS_NUM  4
+#define ASPEED_UARTS_NUM 13
 
 struct AspeedSoCState {
     /*< private >*/
@@ -79,7 +81,7 @@  struct AspeedSoCState {
     AspeedSDHCIState emmc;
     AspeedLPCState lpc;
     AspeedPECIState peci;
-    uint32_t uart_default;
+    SerialMM uart[ASPEED_UARTS_NUM];
     Clock *sysclk;
     UnimplementedDeviceState iomem;
     UnimplementedDeviceState video;
@@ -176,6 +178,7 @@  enum {
 
 qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int dev);
 void aspeed_soc_uart_init(AspeedSoCState *s);
+void aspeed_soc_uart_set_chr(AspeedSoCState *s, int dev, Chardev *chr);
 bool aspeed_soc_dram_init(AspeedSoCState *s, Error **errp);
 void aspeed_mmio_map(AspeedSoCState *s, SysBusDevice *dev, int n, hwaddr addr);
 void aspeed_mmio_map_unimplemented(AspeedSoCState *s, SysBusDevice *dev,