Re: [Qemu-devel] [PATCH 5/6] i.MX: Add i.MX6 SOC implementation.

[Peter Maydell]

On 26 January 2016 at 21:45, Jean-Christophe Dubois <address@hidden> wrote:
> For now we only support the following devices:
> * up to 4 Cortex A9 cores
> * A9 MPCORE (SCU, GIC, TWD)
> * 5 i.MX UARTs
> * 2 EPIT timers
> * 1 GPT timer
> * 7 GPIO controllers
> * 6 SDHC controllers
> * 1 CCM device
> * 1 SRC device
> * various ROM/RAM areas.
>
> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> ---
>  default-configs/arm-softmmu.mak |   1 +
>  hw/arm/Makefile.objs            |   1 +
>  hw/arm/fsl-imx6.c               | 402 ++++++++++++++++++++++++++++++++++++
>  include/hw/arm/fsl-imx6.h       | 447 
> ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 851 insertions(+)
>  create mode 100644 hw/arm/fsl-imx6.c
>  create mode 100644 include/hw/arm/fsl-imx6.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index d9b90a5..ba3a380 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -99,6 +99,7 @@ CONFIG_ALLWINNER_A10_PIT=y
>  CONFIG_ALLWINNER_A10_PIC=y
>  CONFIG_ALLWINNER_A10=y
>
> +CONFIG_FSL_IMX6=y
>  CONFIG_FSL_IMX31=y
>  CONFIG_FSL_IMX25=y
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 2195b60..ac383df 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -15,3 +15,4 @@ obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
>  obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp.o xlnx-ep108.o
>  obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o imx25_pdk.o
>  obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
> +obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o
> diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
> new file mode 100644
> index 0000000..9167392
> --- /dev/null
> +++ b/hw/arm/fsl-imx6.c
> @@ -0,0 +1,402 @@
> +/*
> + * Copyright (c) 2015 Jean-Christophe Dubois <address@hidden>
> + *
> + * i.MX6 SOC emulation.
> + *
> + * Based on hw/arm/fsl-imx31.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; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  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.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "hw/arm/fsl-imx6.h"
> +#include "sysemu/sysemu.h"
> +#include "exec/address-spaces.h"
> +#include "hw/boards.h"
> +#include "sysemu/char.h"
> +
> +static void fsl_imx6_init(Object *obj)
> +{
> +    FslIMX6State *s = FSL_IMX6(obj);
> +    int i;
> +
> +    if (smp_cpus > FSL_IMX6_NUM_CPUS) {
> +        exit(1);

Silently exiting is wrong. If this can only be hit if there's
a bug in another bit of QEMU, you can assert. If it can be hit
if the user gives us some unhelpful command line, you need to
signal the error back to the caller so it can tell the user.

> +    }
> +
> +    for (i = 0; i < smp_cpus; i++) {
> +        object_initialize(&s->cpu[i], sizeof(s->cpu[i]),
> +                          "cortex-a9-" TYPE_ARM_CPU);
> +    }


> +    object_property_set_bool(OBJECT(&s->src), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->src), 0, FSL_IMX6_SRC_ADDR);
> +
> +    /* Initialize all UARTS */

"UARTs"

> +    for (i = 0; i < FSL_IMX6_NUM_UARTS; i++) {
> +        static const struct {
> +            hwaddr addr;
> +            unsigned int irq;
> +        } serial_table[FSL_IMX6_NUM_UARTS] = {
> +            { FSL_IMX6_UART1_ADDR, FSL_IMX6_UART1_IRQ },
> +            { FSL_IMX6_UART2_ADDR, FSL_IMX6_UART2_IRQ },
> +            { FSL_IMX6_UART3_ADDR, FSL_IMX6_UART3_IRQ },
> +            { FSL_IMX6_UART4_ADDR, FSL_IMX6_UART4_IRQ },
> +            { FSL_IMX6_UART5_ADDR, FSL_IMX6_UART5_IRQ },
> +        };
> +
> +        if (i < MAX_SERIAL_PORTS) {
> +            CharDriverState *chr;
> +
> +            chr = serial_hds[i];
> +
> +            if (!chr) {
> +                char label[20];
> +                snprintf(label, sizeof(label), "imx6.uart%d", i);

Please use g_strdup_printf() (and then g_free()) for this sort of
"create a string" use, rather than using a local array that needs
care to keep the size large enough for the contents.

Looks OK otherwise.

thanks
-- PMM