Re: [Qemu-devel] [PATCH v8 09/11] i.MX25: Add the i.MX25 SOC support

[Peter Crosthwaite]

On Mon, Jun 29, 2015 at 1:12 PM, Jean-Christophe Dubois
<address@hidden> wrote:
> For now we do support the foolowing devices:
>   * GPT timers (from i.MX31)
>   * EPIT timers (from i.MX31)
>   * CCM (from i.MX31)
>   * AVIC (from i.MX31)
>   * UART (from i.MX31)
>   * Ethernet FEC port
>   * I2C controller
>
> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> ---
>
> Changes since v1:
>     * not present on v1
>
> Changes since v2:
>     * not present on v2
>
> Changes since v3:
>     * not present on v3
>
> Changes since v4:
>     * not present on v4
>
> Changes since v5:
>     * not present on v5
>
> Changes since v6:
>     * not present on v6
>
> Changes since v7:
>     * Added a SOC specific file for i.MX25
>
>  default-configs/arm-softmmu.mak |   1 +
>  hw/arm/Makefile.objs            |   1 +
>  hw/arm/fsl-imx25.c              | 304 
> ++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/fsl-imx25.h      |  52 +++++++
>  4 files changed, 358 insertions(+)
>  create mode 100644 hw/arm/fsl-imx25.c
>  create mode 100644 include/hw/arm/fsl-imx25.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index c6f509d..057f39c 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_IMX25=y
>  CONFIG_IMX_I2C=y
>
>  CONFIG_XIO3130=y
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index cf346c1..b89c31d 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -13,3 +13,4 @@ obj-y += omap1.o omap2.o strongarm.o
>  obj-$(CONFIG_ALLWINNER_A10) += allwinner-a10.o cubieboard.o
>  obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
>  obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp.o xlnx-ep108.o
> +obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o
> diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
> new file mode 100644
> index 0000000..b2db548
> --- /dev/null
> +++ b/hw/arm/fsl-imx25.c
> @@ -0,0 +1,304 @@
> +/*
> + * Copyright (c) 2013 Jean-Christophe Dubois <address@hidden>
> + *
> + * i.MX25 SOC emulation.
> + *
> + * Based on hw/arm/xlnx-zynqmp.c
> + *
> + * Copyright (C) 2015 Xilinx Inc
> + * Written by Peter Crosthwaite <address@hidden>
> + *

You probably don't need this if you are just borrowing template.

> + *  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-imx25.h"
> +#include "sysemu/sysemu.h"
> +
> +/* Memory map for 3D-Stack Emulation Baseboard:
> + * 0x00000000-0x00003fff 16k ROM              IGNORED
> + * 0x00004000-0x00403fff Reserved             IGNORED
> + * 0x00404000-0x00408fff 20k ROM              IGNORED
> + * 0x00409000-0x0fffffff Reserved             IGNORED
> + * 0x10000000-0x1fffffff Reserved             IGNORED
> + * 0x20000000-0x2fffffff Reserved             IGNORED
> + * 0x30000000-0x3fffffff Reserved             IGNORED
> + * 0x40000000-0x43efffff Reserved             IGNORED
> + * 0x43f00000-0x6fffffff I.MX25 Internal Register Space
> + *   0x43f00000 IO_AREA0
> + *   0x43f80000 I2C0                          EMULATED
> + *   0x43f84000 I2C2                          EMULATED
> + *   0x43f98000 I2C1                          EMULATED
> + *   0x43f90000 UART1                         EMULATED
> + *   0x43f94000 UART2                         EMULATED
> + *   0x43fb0000 UART4                         IGNORED
> + *   0x43fb4000 UART5                         IGNORED
> + *   0x5000c000 UART3                         EMULATED
> + *   0x50038000 FEC                           EMULATED
> + *   0x53f80000 CCM                           EMULATED
> + *   0x53f84000 GPT 4                         EMULATED

So the ones thare are use by the code should just be #defines. This
eliminates repetition of the literal constants in the mmio mappings..

#define GPT_4_ADDR    0x53f83000 /* EMULATED */

For consistency I would just convert the whole table, even the ignored devs.

> + *   0x53f88000 GPT 3                         EMULATED
> + *   0x53f8c000 GPT 2                         EMULATED
> + *   0x53f90000 GPT 1                         EMULATED
> + *   0x53f94000 PIT 1                         EMULATED
> + *   0x53f98000 PIT 2                         EMULATED
> + *   0x53f9c000 GPIO-4                        EMULATED
> + *   0x53fa4000 GPIO-3                        EMULATED
> + *   0x53fcc000 GPIO-1                        EMULATED
> + *   0x53fd0000 GPIO-2                        EMULATED
> + *   0x68000000 ASIC                          EMULATED
> + * 0x78000000-0x7801ffff SRAM                 EMULATED
> + * 0x78020000-0x7fffffff SRAM Aliasing        EMULATED
> + * 0x80000000-0x87ffffff RAM + Alias          EMULATED
> + * 0x90000000-0x9fffffff RAM + Alias          EMULATED
> + * 0xa0000000-0xa7ffffff Flash                IGNORED
> + * 0xa8000000-0xafffffff Flash                IGNORED
> + * 0xb0000000-0xb1ffffff SRAM                 IGNORED
> + * 0xb2000000-0xb3ffffff SRAM                 IGNORED
> + * 0xb4000000-0xb5ffffff CS4                  IGNORED
> + * 0xb6000000-0xb8000fff Reserved             IGNORED
> + * 0xb8001000-0xb8001fff SDRAM CTRL reg       IGNORED
> + * 0xb8002000-0xb8002fff WEIM CTRL reg        IGNORED
> + * 0xb8003000-0xb8003fff M3IF CTRL reg        IGNORED
> + * 0xb8004000-0xb8004fff EMI CTRL reg         IGNORED
> + * 0xb8005000-0xbaffffff Reserved             IGNORED
> + * 0xbb000000-0xbb000fff NAND flash area buf  IGNORED
> + * 0xbb001000-0xbb0011ff NAND flash reserved  IGNORED
> + * 0xbb001200-0xbb001dff Reserved             IGNORED
> + * 0xbb001e00-0xbb001fff NAN flash CTRL reg   IGNORED
> + * 0xbb012000-0xbfffffff Reserved             IGNORED
> + * 0xc0000000-0xffffffff Reserved             IGNORED
> + */
> +
> +static void fsl_imx25_init(Object *obj)
> +{
> +    FslImx25State *s = FSL_IMX25(obj);
> +    int i;
> +
> +    object_initialize(&s->cpu, sizeof(s->cpu), "arm926-" TYPE_ARM_CPU);
> +
> +    object_initialize(&s->avic, sizeof(s->avic), TYPE_IMX_AVIC);
> +    qdev_set_parent_bus(DEVICE(&s->avic), sysbus_get_default());
> +
> +    object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX_CCM);
> +    qdev_set_parent_bus(DEVICE(&s->ccm), sysbus_get_default());
> +
> +    for (i = 0; i < FSL_IMX25_NUM_UARTS; i++) {
> +        if (i >= MAX_SERIAL_PORTS) {
> +            break;
> +        }
> +        object_initialize(&s->uart[i], sizeof(s->uart[i]), TYPE_IMX_SERIAL);
> +        qdev_set_parent_bus(DEVICE(&s->uart[i]), sysbus_get_default());
> +    }
> +
> +    for (i = 0; i < FSL_IMX25_NUM_GPTS; i++) {
> +        object_initialize(&s->gpt[i], sizeof(s->gpt[i]), TYPE_IMX_GPT);
> +        qdev_set_parent_bus(DEVICE(&s->gpt[i]), sysbus_get_default());
> +    }
> +
> +    for (i = 0; i < FSL_IMX25_NUM_EPITS; i++) {
> +        object_initialize(&s->epit[i], sizeof(s->epit[i]), TYPE_IMX_EPIT);
> +        qdev_set_parent_bus(DEVICE(&s->epit[i]), sysbus_get_default());
> +    }
> +
> +    object_initialize(&s->fec, sizeof(s->fec), TYPE_IMX_FEC);
> +    qdev_set_parent_bus(DEVICE(&s->fec), sysbus_get_default());
> +
> +    for (i = 0; i < FSL_IMX25_NUM_I2CS; i++) {
> +        object_initialize(&s->i2c[i], sizeof(s->i2c[i]), TYPE_IMX_I2C);
> +        qdev_set_parent_bus(DEVICE(&s->i2c[i]), sysbus_get_default());
> +    }
> +}
> +
> +static void fsl_imx25_realize(DeviceState *dev, Error **errp)
> +{
> +    FslImx25State *s = FSL_IMX25(dev);
> +    //MemoryRegion *system_memory = get_system_memory();

Remove commented code. (does checkpatch catch this due to CPP comment?)

> +    uint8_t i;
> +    Error *err = NULL;
> +
> +    /* Initialize the CPU */

Code is self documentating, you can drop these. I would drop most of
them except in cases where the code is irrugular, unusual or tricky in
some way.

> +    object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
> +    if (err) {
> +        error_propagate((errp), (err));
> +        return;
> +    }
> +
> +    /* Initialize the PIC */
> +    object_property_set_bool(OBJECT(&s->avic), true, "realized", &err);
> +    if (err) {
> +        error_propagate((errp), (err));
> +        return;
> +    }
> +    /* Connect the PIC interrupt to the CPU */

This comment ...

> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->avic), 0, 0x68000000);

Should go here.

> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->avic), 0,
> +                       qdev_get_gpio_in(DEVICE(&s->cpu), ARM_CPU_IRQ));
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->avic), 1,
> +                       qdev_get_gpio_in(DEVICE(&s->cpu), ARM_CPU_FIQ));
> +
> +    /* Initialize the CCM */
> +    object_property_set_bool(OBJECT(&s->ccm), true, "realized", &err);
> +    if (err) {
> +        error_propagate((errp), (err));
> +        return;
> +    }
> +    /* Map CCM memory */
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->ccm), 0, 0x53f80000);
> +

Constants can be replaced with the macro defs.

> +    /* Initialize all UARTS */
> +    for (i = 0; i < FSL_IMX25_NUM_UARTS; i++) {
> +        static const struct {
> +            hwaddr addr;
> +            unsigned int irq;
> +        } serial_table[FSL_IMX25_NUM_UARTS] = {
> +            { 0x43f90000, 45 },
> +            { 0x43f94000, 32 },
> +            { 0x5000c000, 18 },
> +            { 0x50008000, 5  },
> +            { 0x5002c000, 40 }

And the same.

Alternatively, zynqmp defines these tables up top of the files as
static const. This is to avoid the need for a memory map comment. You
could do the same, pulling these tables up top to replace relevant
sections of the comment.

> +        };
> +
> +        /* Bail out if we exeeded Qemu UART count */

"exceeded" "QEMU"

Regards,
Peter

> +        if (i >= MAX_SERIAL_PORTS) {
> +            break;
> +        }