qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v10 14/21] i.MX: Add SOC support for i.MX31


From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [PATCH v10 14/21] i.MX: Add SOC support for i.MX31
Date: Mon, 6 Jul 2015 00:18:16 -0700

Needs a short paragraph. List out the initally support peripherals.

On Sun, Jul 5, 2015 at 5:05 PM, Jean-Christophe Dubois
<address@hidden> wrote:
> 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:
>     * not present on v7
>
> Changes since v8:
>     * use defines instead of hardcoded values for IRQ and ADDR
>     * Add i.MX31 SOC support
>
> Changes since v9:
>     * no change.
>
>  default-configs/arm-softmmu.mak |   2 +
>  hw/arm/Makefile.objs            |   1 +
>  hw/arm/fsl-imx31.c              | 219 
> ++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/fsl-imx31.h      |  99 ++++++++++++++++++
>  4 files changed, 321 insertions(+)
>  create mode 100644 hw/arm/fsl-imx31.c
>  create mode 100644 include/hw/arm/fsl-imx31.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 74f1db3..3f86e7e 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -98,6 +98,8 @@ CONFIG_ALLWINNER_A10_PIT=y
>  CONFIG_ALLWINNER_A10_PIC=y
>  CONFIG_ALLWINNER_A10=y
>
> +CONFIG_FSL_IMX31=y
> +
>  CONFIG_XIO3130=y
>  CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index cf346c1..f35f731 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_IMX31) += fsl-imx31.o
> diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
> new file mode 100644
> index 0000000..809070b
> --- /dev/null
> +++ b/hw/arm/fsl-imx31.c
> @@ -0,0 +1,219 @@
> +/*
> + * Copyright (c) 2013 Jean-Christophe Dubois <address@hidden>
> + *
> + * i.MX31 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-imx31.h"
> +#include "sysemu/sysemu.h"
> +#include "exec/address-spaces.h"
> +
> +static void fsl_imx31_init(Object *obj)
> +{
> +    FslImx31State *s = FSL_IMX31(obj);
> +    int i;
> +
> +    object_initialize(&s->cpu, sizeof(s->cpu), "arm1136-" 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_IMX31_NUM_UARTS; i++) {
> +        if (i >= MAX_SERIAL_PORTS) {
> +            break;
> +        }

You don't need this bail-out. For SoCs, the device should always be
created. If QEMU can't attach the serial port to anything that is ok.
If the UART model can't handle being un-attached (chr == NULL) it
should be patched (but thats out of scope for the moment I think).

> +        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_IMX31_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_IMX31_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());
> +    }
> +}
> +
> +static void fsl_imx31_realize(DeviceState *dev, Error **errp)
> +{
> +    FslImx31State *s = FSL_IMX31(dev);
> +    uint16_t i;
> +    Error *err = NULL;
> +
> +    /* Initialize the CPU */

Self documented, drop comment.

> +    object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
> +    if (err) {
> +        error_propagate((errp), (err));

parentheses not needed (x2). Happens a few times below as well.

> +        return;
> +    }
> +
> +    /* Initialize the PIC */

What is PIC? I would just say 'interrupt controller" (if you are going
for programmable-interrupt-controller) or drop the comment.

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

"interrupts" (you have both IRQ and FIQ).

This comment...

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

... goes here. Or just drop it. It is self documented.

> +    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);

Missed address macroification.

> +
> +    /* Initialize all UARTS */
> +    for (i = 0; i < FSL_IMX31_NUM_UARTS; i++) {
> +        static const struct {
> +            hwaddr addr;
> +            unsigned int irq;
> +        } serial_table[FSL_IMX31_NUM_UARTS] = {
> +            { FSL_IMX31_UART1_ADDR, FSL_IMX31_UART1_IRQ },
> +            { FSL_IMX31_UART2_ADDR, FSL_IMX31_UART2_IRQ },
> +        };
> +
> +        /* Bail out if we exeeded Qemu UART count */

"exceeded" "QEMU"

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

Is this where the chr property should be set?

> +        /* Initialize the UART */
> +        object_property_set_bool(OBJECT(&s->uart[i]), true, "realized", 
> &err);
> +        if (err) {
> +            error_propagate((errp), (err));
> +            return;
> +        }
> +        /* Map UART memory */

Self-documented - drop comment.

> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->uart[i]), 0, 
> serial_table[i].addr);
> +        /* Connet UART IRQ to PIC */

Self-documented - drop comment.

> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart[i]), 0,
> +                           qdev_get_gpio_in(DEVICE(&s->avic),
> +                                            serial_table[i].irq));

I'd pull the GPIO in to a local variable to save on the double 80 char
wrap of mixed indents.

> +    }
> +
> +    /* Initialize all GPT timers */
> +    for (i = 0; i < FSL_IMX31_NUM_GPTS; i++) {
> +        static const struct {
> +            hwaddr addr;
> +            unsigned int irq;
> +        } gpt_table[FSL_IMX31_NUM_GPTS] = {
> +            { FSL_IMX31_GPT_ADDR, FSL_IMX31_GPT_IRQ }
> +        };
> +
> +        s->gpt[i].ccm = DEVICE(&s->ccm);
> +

We should consider making this a QOM link, but is out of scope of
series so no change needed.

> +        /* Initialize the GPT */
> +        object_property_set_bool(OBJECT(&s->gpt[i]), true, "realized", &err);
> +        if (err) {
> +            error_propagate((errp), (err));
> +            return;
> +        }
> +        /* Map GPT memory */

Self-documented - drop comment.

> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpt[i]), 0, gpt_table[i].addr);
> +        /* Connet GPT IRQ to PIC */

Self-documented - drop comment.

> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpt[i]), 0,
> +                           qdev_get_gpio_in(DEVICE(&s->avic),
> +                                            gpt_table[i].irq));
> +    }
> +
> +    /* Initialize all EPIT timers */

Self-documented - drop comment.

> +    for (i = 0; i < FSL_IMX31_NUM_EPITS; i++) {
> +        static const struct {
> +            hwaddr addr;
> +            unsigned int irq;
> +        } epit_table[FSL_IMX31_NUM_EPITS] = {
> +            { FSL_IMX31_EPIT1_ADDR, FSL_IMX31_EPIT1_IRQ },
> +            { FSL_IMX31_EPIT2_ADDR, FSL_IMX31_EPIT2_IRQ },
> +        };
> +
> +        s->epit[i].ccm = DEVICE(&s->ccm);
> +
> +        /* Initialize the EPIT */

Self-documented - drop comment.

> +        object_property_set_bool(OBJECT(&s->epit[i]), true, "realized", 
> &err);
> +        if (err) {
> +            error_propagate((errp), (err));
> +            return;
> +        }
> +        /* Map EPIT memory */

Self-documented - drop comment.


> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->epit[i]), 0, epit_table[i].addr);
> +        /* Connet EPIT IRQ to PIC */
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->epit[i]), 0,
> +                           qdev_get_gpio_in(DEVICE(&s->avic),
> +                                            epit_table[i].irq));
> +    }
> +
> +    /* On a real system, the first 16k is a `secure boot rom' */
> +    memory_region_init_rom_device(&s->secure_rom, NULL, NULL, NULL,
> +                                  "imx31.secure_rom",
> +                                  FSL_IMX31_SECURE_ROM_SIZE, &err);
> +    memory_region_add_subregion(get_system_memory(), 
> FSL_IMX31_SECURE_ROM_ADDR,
> +                                &s->secure_rom);
> +
> +    /* There is also a 16k ROM */
> +    memory_region_init_rom_device(&s->rom, NULL, NULL, NULL, "imx31.rom",
> +                                  FSL_IMX31_ROM_SIZE, &err);
> +    memory_region_add_subregion(get_system_memory(), FSL_IMX31_ROM_ADDR,
> +                                &s->rom);
> +
> +    /* initialize internal RAM (16 KB) */
> +    memory_region_init_ram(&s->iram, NULL, "imx31.iram", FSL_IMX31_IRAM_SIZE,
> +                           &error_abort);
> +    vmstate_register_ram_global(&s->iram);
> +    memory_region_add_subregion(get_system_memory(), FSL_IMX31_IRAM_ADDR,
> +                                &s->iram);

I think there is a new feature for this,
memory_region_allocate_system_memory. Check xlnx-ep108.

> +
> +    /* internal RAM (16 KB) is aliased over 256 MB - 16 KB */
> +    memory_region_init_alias(&s->iram_alias, NULL, "imx31.iram_alias",
> +                             &s->iram, 0, FSL_IMX31_IRAM_ALIAS_SIZE);
> +    memory_region_add_subregion(get_system_memory(), 
> FSL_IMX31_IRAM_ALIAS_ADDR,
> +                                &s->iram_alias);
> +}
> +
> +static void fsl_imx31_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = fsl_imx31_realize;
> +}
> +
> +static const TypeInfo fsl_imx31_type_info = {
> +    .name = TYPE_FSL_IMX31,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(FslImx31State),
> +    .instance_init = fsl_imx31_init,
> +    .class_init = fsl_imx31_class_init,
> +};
> +
> +static void fsl_imx31_register_types(void)
> +{
> +    type_register_static(&fsl_imx31_type_info);
> +}
> +
> +type_init(fsl_imx31_register_types)
> diff --git a/include/hw/arm/fsl-imx31.h b/include/hw/arm/fsl-imx31.h
> new file mode 100644
> index 0000000..f1bb299
> --- /dev/null
> +++ b/include/hw/arm/fsl-imx31.h
> @@ -0,0 +1,99 @@
> +/*
> + * Freescale i.MX31 SoC emulation
> + *
> + * Copyright (C) 2015 Jean-Christophe Dubois <address@hidden>
> + *
> + * 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.
> + */
> +
> +#ifndef FSL_IMX31_H
> +#define FSL_IMX31_H
> +
> +#include "hw/arm/arm.h"
> +#include "hw/intc/imx_avic.h"
> +#include "hw/misc/imx_ccm.h"
> +#include "hw/char/imx_serial.h"
> +#include "hw/timer/imx_gpt.h"
> +#include "hw/timer/imx_epit.h"
> +#include "exec/memory.h"
> +
> +#define TYPE_FSL_IMX31 "fsl,imx31"
> +#define FSL_IMX31(obj) OBJECT_CHECK(FslImx31State, (obj), TYPE_FSL_IMX31)
> +
> +#define FSL_IMX31_NUM_UARTS 2
> +#define FSL_IMX31_NUM_GPTS 1

Can/should we de-arrayify a singleton device? Are you planning code
share with more SoCs that may have multiple of these?

> +#define FSL_IMX31_NUM_EPITS 2
> +
> +typedef struct {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +
> +    /*< public >*/
> +    ARMCPU         cpu;
> +    IMXAVICState   avic;
> +    IMXCCMState    ccm;
> +    IMXSerialState uart[FSL_IMX31_NUM_UARTS];
> +    IMXGPTState    gpt[FSL_IMX31_NUM_GPTS];
> +    IMXEPITState   epit[FSL_IMX31_NUM_EPITS];
> +    MemoryRegion   secure_rom;
> +    MemoryRegion   rom;
> +    MemoryRegion   iram;
> +    MemoryRegion   iram_alias;
> +} FslImx31State;

FslIMX21State

> +
> +#define FSL_IMX31_SECURE_ROM_ADDR      0x00000000
> +#define FSL_IMX31_SECURE_ROM_SIZE      0x4000
> +#define FSL_IMX31_ROM_ADDR             0x00404000
> +#define FSL_IMX31_ROM_SIZE             0x4000
> +#define FSL_IMX31_IRAM_ALIAS_ADDR      0x10000000
> +#define FSL_IMX31_IRAM_ALIAS_SIZE      0xFFC0000
> +#define FSL_IMX31_IRAM_ADDR            0x1FFFC000
> +#define FSL_IMX31_IRAM_SIZE            0x4000
> +#define FSL_IMX31_UART1_ADDR           0x43F90000
> +#define FSL_IMX31_UART1_SIZE           0x4000
> +#define FSL_IMX31_UART2_ADDR           0x43F94000
> +#define FSL_IMX31_UART2_SIZE           0x4000
> +#define FSL_IMX31_CCM_ADDR             0x53F80000
> +#define FSL_IMX31_CCM_SIZE             0x4000
> +#define FSL_IMX31_GPT_ADDR             0x53F90000
> +#define FSL_IMX31_GPT_SIZE             0x4000
> +#define FSL_IMX31_EPIT1_ADDR           0x53F94000
> +#define FSL_IMX31_EPIT1_SIZE           0x4000
> +#define FSL_IMX31_EPIT2_ADDR           0x53F98000
> +#define FSL_IMX31_EPIT2_SIZE           0x4000
> +#define FSL_IMX31_AVIC_ADDR            0x68000000
> +#define FSL_IMX31_AVIC_SIZE            0x100
> +#define FSL_IMX31_SDRAM0_ADDR          0x80000000
> +#define FSL_IMX31_SDRAM0_SIZE          0x10000000
> +#define FSL_IMX31_SDRAM1_ADDR          0x90000000
> +#define FSL_IMX31_SDRAM1_SIZE          0x10000000
> +#define FSL_IMX31_FLASH0_ADDR          0xA0000000
> +#define FSL_IMX31_FLASH0_SIZE          0x8000000
> +#define FSL_IMX31_FLASH1_ADDR          0xA8000000
> +#define FSL_IMX31_FLASH1_SIZE          0x8000000
> +#define FSL_IMX31_CS2_ADDR             0xB0000000
> +#define FSL_IMX31_CS2_SIZE             0x2000000
> +#define FSL_IMX31_CS3_ADDR             0xB2000000
> +#define FSL_IMX31_CS3_SIZE             0x2000000
> +#define FSL_IMX31_CS4_ADDR             0xB4000000
> +#define FSL_IMX31_CS4_SIZE             0x2000000
> +#define FSL_IMX31_CS5_ADDR             0xB6000000
> +#define FSL_IMX31_CS5_SIZE             0x2000000
> +#define FSL_IMX31_NAND_ADDR            0xB8000000
> +#define FSL_IMX31_NAND_SIZE            0x1000
> +
> +#define FSL_IMX31_EPIT2_IRQ            27
> +#define FSL_IMX31_EPIT1_IRQ            28
> +#define FSL_IMX31_GPT_IRQ              29
> +#define FSL_IMX31_UART2_IRQ            32
> +#define FSL_IMX31_UART1_IRQ            45
> +

FWIW, I prefered these memory maps in the C file to minimise the
header to just the embeddable state, but it could go either way. You
could in-theory use these macros from the header to drive SoC-level
tests I guess. No change needed.

> +#endif // FSL_IMX31_H

C++ style comment

Regards,
Peter

> --
> 2.1.4
>
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]