Re: [Qemu-devel] [PATCH 16/17] i.MX: Add i.MX7 SOC implementation.

[Peter Maydell]

On 18 September 2017 at 20:50, Andrey Smirnov <address@hidden> wrote:
> For now we only support the following devices:
>     * up to 2 Cortex A9 cores (SMP works with PSCI)
>     * A7 MPCORE (identical to A15 MPCORE)
>     * 7 i.MX UARTs
>     * 1 CCM device
>     * 2 Ethernet controllers (FEC)
>     * 3 SD controllers (USDHC)
>     * 1 SNVS device
>     * 1 WDT device
>
> Cc: Peter Maydell <address@hidden>
> Cc: address@hidden
> Cc: address@hidden
> Cc: address@hidden
> Signed-off-by: Andrey Smirnov <address@hidden>
> ---
>  default-configs/arm-softmmu.mak |   1 +
>  hw/arm/Makefile.objs            |   2 +
>  hw/arm/fsl-imx7.c               | 327 
> ++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/fsl-imx7.h       | 114 ++++++++++++++
>  4 files changed, 444 insertions(+)
>  create mode 100644 hw/arm/fsl-imx7.c
>  create mode 100644 include/hw/arm/fsl-imx7.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index bbdd3c1d8b..98396a3ad2 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -118,6 +118,7 @@ CONFIG_ALLWINNER_A10=y
>  CONFIG_FSL_IMX6=y
>  CONFIG_FSL_IMX31=y
>  CONFIG_FSL_IMX25=y
> +CONFIG_FSL_IMX7=y
>
>  CONFIG_IMX_I2C=y
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index a2e56ecaae..33f6051ae3 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -19,3 +19,5 @@ obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
>  obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
>  obj-$(CONFIG_ASPEED_SOC) += aspeed_soc.o aspeed.o
>  obj-$(CONFIG_MPS2) += mps2.o
> +obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o
> +
> diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
> new file mode 100644
> index 0000000000..bd01bb7f59
> --- /dev/null
> +++ b/hw/arm/fsl-imx7.c
> @@ -0,0 +1,327 @@
> +/*
> + * Copyright (c) 2017, Impinj, Inc.
> + *
> + * i.MX7 SoC definitions
> + *
> + * Author: Andrey Smirnov <address@hidden>
> + *
> + * Based on hw/arm/fsl-imx6.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.

We pretty strongly prefer GPL-2-or-later for new code, not 2-only.

> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +#include "hw/arm/fsl-imx7.h"
> +#include "sysemu/sysemu.h"
> +#include "qemu/error-report.h"
> +
> +#define NAME_SIZE 20
> +
> +#define for_each_cpu(i)     for (i = 0; i < smp_cpus; i++)

Just open-code this, please.

> +
> +static void fsl_imx7_init(Object *obj)
> +{
> +    BusState *sysbus = sysbus_get_default();
> +    FslIMX7State *s = FSL_IMX7(obj);
> +    char name[NAME_SIZE];
> +    int i;
> +
> +    if (smp_cpus > FSL_IMX7_NUM_CPUS) {
> +        error_report("%s: Only %d CPUs are supported (%d requested)",
> +                     TYPE_FSL_IMX7, FSL_IMX7_NUM_CPUS, smp_cpus);
> +        exit(1);
> +    }
> +
> +    for_each_cpu(i) {
> +        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
> +                          "cortex-a7-" TYPE_ARM_CPU);
> +        snprintf(name, NAME_SIZE, "cpu%d", i);
> +        object_property_add_child(obj, name, OBJECT(&s->cpu[i]),
> +                                  &error_fatal);
> +    }
> +
> +    /*
> +     * A7MPCORE
> +     */
> +    object_initialize(&s->a7mpcore, sizeof(s->a7mpcore), 
> TYPE_A15MPCORE_PRIV);

Stealing the A15MPCORE device for a7 is kind of ugly, but I can't
decide what would be better instead...

> +    qdev_set_parent_bus(DEVICE(&s->a7mpcore), sysbus);
> +    object_property_add_child(obj, "a7mpcore",
> +                              OBJECT(&s->a7mpcore), &error_fatal);
> +
> +    /*
> +     * CCM
> +     */
> +    object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX7_CCM);
> +    qdev_set_parent_bus(DEVICE(&s->ccm), sysbus);
> +    object_property_add_child(obj, "ccm", OBJECT(&s->ccm), &error_fatal);
> +
> +    /*
> +     * UART
> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_UARTS; i++) {
> +            object_initialize(&s->uart[i], sizeof(s->uart[i]), 
> TYPE_IMX_SERIAL);
> +            qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus);
> +            snprintf(name, NAME_SIZE, "uart%d", i);
> +            object_property_add_child(obj, name, OBJECT(&s->uart[i]),
> +                                      &error_fatal);
> +    }
> +
> +    /*
> +     * Ethernet
> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
> +            object_initialize(&s->eth[i], sizeof(s->eth[i]), TYPE_IMX_ENET);
> +            qdev_set_parent_bus(DEVICE(&s->eth[i]), sysbus);
> +            snprintf(name, NAME_SIZE, "eth%d", i);
> +            object_property_add_child(obj, name, OBJECT(&s->eth[i]),
> +                                      &error_fatal);
> +    }
> +
> +    /*
> +     * SDHCI
> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
> +            object_initialize(&s->usdhc[i], sizeof(s->usdhc[i]),
> +                              TYPE_IMX_USDHC);
> +            qdev_set_parent_bus(DEVICE(&s->usdhc[i]), sysbus);
> +            snprintf(name, NAME_SIZE, "usdhc%d", i);
> +            object_property_add_child(obj, name, OBJECT(&s->usdhc[i]),
> +                                      &error_fatal);
> +    }
> +
> +    /*
> +     * SNVS
> +     */
> +    object_initialize(&s->snvs, sizeof(s->snvs), TYPE_IMX7_SNVS);
> +    qdev_set_parent_bus(DEVICE(&s->snvs), sysbus);
> +    object_property_add_child(obj, "snvs", OBJECT(&s->snvs), &error_fatal);
> +
> +    /*
> +     * Watchdog
> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_WDTS; i++) {
> +            object_initialize(&s->wdt[i], sizeof(s->wdt[i]), TYPE_IMX2_WDT);
> +            qdev_set_parent_bus(DEVICE(&s->wdt[i]), sysbus);
> +            snprintf(name, NAME_SIZE, "wdt%d", i);
> +            object_property_add_child(obj, name, OBJECT(&s->wdt[i]),
> +                                      &error_fatal);
> +    }
> +}
> +
> +static void fsl_imx7_realize(DeviceState *dev, Error **errp)
> +{
> +    FslIMX7State *s = FSL_IMX7(dev);
> +    Object *o;
> +    int i;
> +    qemu_irq irq;
> +
> +    for_each_cpu(i) {
> +       o = OBJECT(&s->cpu[i]);
> +
> +        object_property_set_int(o, QEMU_PSCI_CONDUIT_SMC,
> +                                "psci-conduit", &error_abort);
> +
> +        object_property_set_bool(o, false, "has_el3", &error_abort);

Does this SoC's CPU really not have EL3?

> +
> +        /* On uniprocessor, the CBAR is set to 0 */
> +        if (smp_cpus > 1) {
> +            object_property_set_int(o, FSL_IMX7_A7MPCORE_ADDR,
> +                                    "reset-cbar", &error_abort);
> +        }
> +
> +        if (i) {
> +            /* Secondary CPUs start in PSCI powered-down state */
> +            object_property_set_bool(o, true,
> +                                     "start-powered-off", &error_abort);
> +        }
> +
> +        object_property_set_bool(o, true, "realized", &error_abort);
> +    }
> +
> +    /*
> +     * A7MPCORE
> +     */
> +    object_property_set_int(OBJECT(&s->a7mpcore), smp_cpus, "num-cpu",
> +                            &error_abort);
> +    object_property_set_int(OBJECT(&s->a7mpcore),
> +                            FSL_IMX7_MAX_IRQ + GIC_INTERNAL,
> +                            "num-irq", &error_abort);
> +
> +    object_property_set_bool(OBJECT(&s->a7mpcore), true, "realized",
> +                             &error_abort);
> +
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->a7mpcore), 0, FSL_IMX7_A7MPCORE_ADDR);
> +
> +    for_each_cpu(i) {
> +        SysBusDevice *sbd = SYS_BUS_DEVICE(&s->a7mpcore);
> +        DeviceState  *d   = DEVICE(qemu_get_cpu(i));
> +
> +        irq = qdev_get_gpio_in(d, ARM_CPU_IRQ);
> +        sysbus_connect_irq(sbd, i, irq);
> +        irq = qdev_get_gpio_in(d, ARM_CPU_FIQ);
> +        sysbus_connect_irq(sbd, i + smp_cpus, irq);
> +    }
> +
> +    /*
> +     * CCM
> +     */
> +    object_property_set_bool(OBJECT(&s->ccm), true, "realized", 
> &error_abort);
> +
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->ccm), 0, FSL_IMX7_CCM_ADDR);
> +
> +    /*
> +     * UART
> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_UARTS; i++) {
> +        static const hwaddr FSL_IMX7_UARTn_ADDR[FSL_IMX7_NUM_UARTS] = {
> +            FSL_IMX7_UART1_ADDR,
> +            FSL_IMX7_UART2_ADDR,
> +            FSL_IMX7_UART3_ADDR,
> +            FSL_IMX7_UART4_ADDR,
> +            FSL_IMX7_UART5_ADDR,
> +            FSL_IMX7_UART6_ADDR,
> +            FSL_IMX7_UART7_ADDR,
> +        };
> +
> +        static const int FSL_IMX7_UARTn_IRQ[FSL_IMX7_NUM_UARTS] = {
> +            FSL_IMX7_UART1_IRQ,
> +            FSL_IMX7_UART2_IRQ,
> +            FSL_IMX7_UART3_IRQ,
> +            FSL_IMX7_UART4_IRQ,
> +            FSL_IMX7_UART5_IRQ,
> +            FSL_IMX7_UART6_IRQ,
> +            FSL_IMX7_UART7_IRQ,
> +        };
> +
> +
> +        if (i < MAX_SERIAL_PORTS) {
> +            Chardev *chr;
> +            chr = serial_hds[i];
> +
> +            if (!chr) {
> +                char *label = g_strdup_printf("imx7.uart%d", i + 1);
> +                chr = qemu_chr_new(label, "null");
> +                g_free(label);
> +                serial_hds[i] = chr;
> +            }

I think the conclusion we've come to about chardevs is that the
UART should handle being passed a NULL chardev, rather than the
board code having to create a dummy chardev to pass to it.

> +
> +            qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
> +        }
> +
> +        object_property_set_bool(OBJECT(&s->uart[i]), true, "realized",
> +                                 &error_abort);
> +
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->uart[i]), 0, 
> FSL_IMX7_UARTn_ADDR[i]);
> +
> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_UARTn_IRQ[i]);
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart[i]), 0, irq);
> +    }
> +
> +    /*
> +     * Etherenet

Typo: "Ethernet"

> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
> +        static const hwaddr FSL_IMX7_ENETn_ADDR[FSL_IMX7_NUM_ETHS] = {
> +            FSL_IMX7_ENET1_ADDR,
> +            FSL_IMX7_ENET2_ADDR,
> +        };
> +
> +        qdev_set_nic_properties(DEVICE(&s->eth[i]), &nd_table[i]);
> +        object_property_set_bool(OBJECT(&s->eth[i]), true, "realized",
> +                                 &error_abort);
> +
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->eth[i]), 0, 
> FSL_IMX7_ENETn_ADDR[i]);
> +
> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_ENET_IRQ(i, 
> 0));
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->eth[i]), 0, irq);
> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_ENET_IRQ(i, 
> 3));
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->eth[i]), 1, irq);
> +    }
> +
> +    /*
> +     * USDHC
> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
> +        static const hwaddr FSL_IMX7_USDHCn_ADDR[FSL_IMX7_NUM_USDHCS] = {
> +            FSL_IMX7_USDHC1_ADDR,
> +            FSL_IMX7_USDHC2_ADDR,
> +            FSL_IMX7_USDHC3_ADDR,
> +        };
> +
> +        static const int FSL_IMX7_USDHCn_IRQ[FSL_IMX7_NUM_USDHCS] = {
> +            FSL_IMX7_USDHC1_IRQ,
> +            FSL_IMX7_USDHC2_IRQ,
> +            FSL_IMX7_USDHC3_IRQ,
> +        };
> +
> +        object_property_set_bool(OBJECT(&s->usdhc[i]), true, "realized",
> +                                 &error_abort);
> +
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->usdhc[i]), 0,
> +                        FSL_IMX7_USDHCn_ADDR[i]);
> +
> +        irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_USDHCn_IRQ[i]);
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->usdhc[i]), 0, irq);
> +    }
> +
> +    /*
> +     * SNVS
> +     */
> +    object_property_set_bool(OBJECT(&s->snvs), true, "realized", 
> &error_abort);
> +
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->snvs), 0, FSL_IMX7_SNVS_ADDR);
> +
> +    /*
> +     * Watchdog
> +     */
> +    for (i = 0; i < FSL_IMX7_NUM_WDTS; i++) {
> +        static const hwaddr FSL_IMX7_WDOGn_ADDR[FSL_IMX7_NUM_WDTS] = {
> +            FSL_IMX7_WDOG1_ADDR,
> +            FSL_IMX7_WDOG2_ADDR,
> +            FSL_IMX7_WDOG3_ADDR,
> +            FSL_IMX7_WDOG4_ADDR,
> +        };
> +
> +        object_property_set_bool(OBJECT(&s->wdt[i]), true, "realized",
> +                                 &error_abort);
> +
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->wdt[i]), 0, 
> FSL_IMX7_WDOGn_ADDR[i]);
> +    }
> +}
> +
> +static void fsl_imx7_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = fsl_imx7_realize;
> +
> +    /*
> +     * Reason: creates an ARM CPU, thus use after free(), see
> +     * arm_cpu_class_init()
> +     */

I think this is the wrong reason (we've fixed that problem, IIRC).
You do want this not to be user-creatable, but the reason is
/* Reason: uses serial_hds and nd_table in realize() */
(compare the reason text in the aspeed/allwinner/digic/xlnx-zynqmp
SoC device objects).

> +    dc->user_creatable = false;
> +    dc->desc = "i.MX7 SOC";
> +}
> +
> +static const TypeInfo fsl_imx7_type_info = {
> +    .name = TYPE_FSL_IMX7,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(FslIMX7State),
> +    .instance_init = fsl_imx7_init,
> +    .class_init = fsl_imx7_class_init,
> +};
> +
> +static void fsl_imx7_register_types(void)
> +{
> +    type_register_static(&fsl_imx7_type_info);
> +}
> +type_init(fsl_imx7_register_types)

thanks
-- PMM