Re: [Qemu-devel] [PATCH v3 01/20] arm: add Faraday a360 SoC platform support

[Igor Mitsyanko]

On 02/06/2013 01:45 PM, Kuo-Jung Su wrote:
> From: Kuo-Jung Su<address@hidden>
>
> The Faraday A360 EVB is a Faraday SoC platform evaluation board used for
> Faraday IP functional verification based on the well-known ARM AMBA 2.0
> architecture.
>
> Signed-off-by: Kuo-Jung Su<address@hidden>
> ---
>   hw/arm/Makefile.objs      |    1 +
>   hw/arm/faraday.h          |   52 ++++++++++++++++++++
>   hw/arm/faraday_a360.c     |  118 
> +++++++++++++++++++++++++++++++++++++++++++++
>   hw/arm/faraday_a360_pmu.c |  102 +++++++++++++++++++++++++++++++++++++++
>   4 files changed, 273 insertions(+)
>   create mode 100644 hw/arm/faraday.h
>   create mode 100644 hw/arm/faraday_a360.c
>   create mode 100644 hw/arm/faraday_a360_pmu.c
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 6d049e7..59d7023 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -33,3 +33,4 @@ obj-y += kzm.o
>   obj-$(CONFIG_FDT) += ../device_tree.o
>
>   obj-y := $(addprefix ../,$(obj-y))
> +obj-y += faraday_a360.o faraday_a360_pmu.o
> diff --git a/hw/arm/faraday.h b/hw/arm/faraday.h
> new file mode 100644
> index 0000000..25bb056
> --- /dev/null
> +++ b/hw/arm/faraday.h
> @@ -0,0 +1,52 @@
> +/*
> + * Faraday SoC platform support.
> + *
> + * Copyright (c) 2013 Faraday Technology
> + * Written by Kuo-Jung Su<address@hidden>
> + *
> + * This code is licensed under the GNU GPL v2.
> + */
> +#ifndef HW_ARM_FARADAY_H
> +#define HW_ARM_FARADAY_H
> +
> +#include <hw/flash.h>

Its not a system header, you could just use "" instead of <>.

> +
> +#ifdef DEBUG_FARADAY
> +#define DPRINTF(fmt, ...) \
> +    do { printf("faraday: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) \
> +    do { } while (0)
> +#endif
> +
> +typedef struct FaradayMachState {
> +    ARMCPU       *cpu;
> +    DeviceState  *scu;
> +    DeviceState  *ahbc;
> +    DeviceState  *ddrc;
> +    DeviceState  *hdma[2];  /* AHB DMA */
> +    DeviceState  *pdma[2];  /* APB DMA */
> +    i2c_bus      *i2c[2];
> +
> +    MemoryRegion *as;
> +    MemoryRegion *ram;
> +    MemoryRegion *ram_alias;
> +    pflash_t     *rom;
> +    MemoryRegion *sram;
> +
> +    uint32_t ahb_slave4;    /* AHB slave 4 default value */
> +    uint32_t ahb_slave6;    /* AHB slave 6 default value */
> +    uint32_t ahb_remapped:1;
> +    uint32_t ddr_inited:1;
> +} FaradayMachState;
> +
> +/* ftintc020.c */
> +qemu_irq *ftintc020_init(hwaddr base, ARMCPU *cpu);
> +
> +/* ftgmac100.c */
> +void ftgmac100_init(NICInfo *nd, uint32_t base, qemu_irq irq);
> +
> +/* ftmac110.c */
> +void ftmac110_init(NICInfo *nd, uint32_t base, qemu_irq irq);

I understand that its not important, but it doesn't look pretty to have
declarations without definitions. Maybe it'd be better
to add these declaration later, together with function implementations?


> +
> +#endif
> diff --git a/hw/arm/faraday_a360.c b/hw/arm/faraday_a360.c
> new file mode 100644
> index 0000000..a81b6fb
> --- /dev/null
> +++ b/hw/arm/faraday_a360.c
> @@ -0,0 +1,118 @@
> +/*
> + * Faraday A360 Evalution Board
> + *
> + * Copyright (c) 2012 Faraday Technology
> + * Written by Dante Su<address@hidden>
> + *
> + * This code is licensed under GNU GPL v2+.
> + */
> +
> +#include <hw/sysbus.h>
> +#include <hw/arm-misc.h>
> +#include <hw/devices.h>
> +#include <hw/i2c.h>
> +#include <hw/boards.h>
> +#include <hw/flash.h>
> +#include <hw/serial.h>
> +#include <hw/ssi.h>
> +#include <net/net.h>
> +#include <sysemu/sysemu.h>
> +#include <sysemu/blockdev.h>
> +#include <exec/address-spaces.h>
> +
> +#include "faraday.h"
> +
> +typedef FaradayMachState    A360State;
> +
> +/* Board init. */
> +static void
> +a360_device_init(A360State *s)
> +{
> +    /* Serial (FTUART010 which is 16550A compatible) */
> +    if (serial_hds[0]) {
> +        serial_mm_init(s->as,
> +                       0x98200000,
> +                       2,
> +                       NULL,
> +                       18432000 / 16,
> +                       serial_hds[0],
> +                       DEVICE_LITTLE_ENDIAN);
> +    }
> +    if (serial_hds[1]) {
> +        serial_mm_init(s->as,
> +                       0x98300000,
> +                       2,
> +                       NULL,
> +                       18432000 / 16,
> +                       serial_hds[1],
> +                       DEVICE_LITTLE_ENDIAN);
> +    }
> +
> +    /* pmu */
> +    sysbus_create_simple("a360.pmu", 0x98100000, NULL);
> +}
> +
> +static void
> +a360_board_init(QEMUMachineInitArgs *args)
> +{
> +    struct arm_boot_info *bi = NULL;
> +    A360State *s = g_new(A360State, 1);
> +
> +    s->as = get_system_memory();
> +    s->ram  = g_new(MemoryRegion, 1);
> +
> +    /* CPU */
> +    if (!args->cpu_model) {
> +        args->cpu_model = "fa626te";
> +    }
> +
> +    s->cpu = cpu_arm_init(args->cpu_model);
> +    if (!s->cpu) {
> +        args->cpu_model = "arm926";
> +        s->cpu = cpu_arm_init(args->cpu_model);
> +        if (!s->cpu) {
> +            hw_error("a360: Unable to find CPU definition\n");
> +            exit(1);
> +        }
> +    }
> +
> +    /* A360 supports upto 1GB ram space */
> +    if (args->ram_size > 0x40000000) {
> +        args->ram_size = 0x40000000;
> +    }
> +
> +    /* RAM Init */
> +    memory_region_init_ram(s->ram, "a360.ram", args->ram_size);
> +    vmstate_register_ram_global(s->ram);
> +
> +    a360_device_init(s);
> +
> +    /* Prepare for direct boot from linux kernel or u-boot elf */
> +    bi = g_new0(struct arm_boot_info, 1);
> +
> +    /* RAM Address Binding */
> +    memory_region_add_subregion(s->as, 0x00000000, s->ram);
> +
> +    /* Boot Info */
> +    bi->ram_size = args->ram_size;
> +    bi->kernel_filename = args->kernel_filename;
> +    bi->kernel_cmdline = args->kernel_cmdline;
> +    bi->initrd_filename = args->initrd_filename;
> +    bi->board_id = 0xa360;
> +    arm_load_kernel(s->cpu, bi);
> +}
> +
> +static QEMUMachine a360_machine = {
> +    .name = "a360",
> +    .desc = "Faraday A360 (fa626te)",
> +    .init = a360_board_init,
> +    DEFAULT_MACHINE_OPTIONS,
> +};
> +
> +static void
> +a360_machine_init(void)
> +{
> +    qemu_register_machine(&a360_machine);
> +}
> +
> +machine_init(a360_machine_init);
> diff --git a/hw/arm/faraday_a360_pmu.c b/hw/arm/faraday_a360_pmu.c
> new file mode 100644
> index 0000000..dc8b749
> --- /dev/null
> +++ b/hw/arm/faraday_a360_pmu.c
> @@ -0,0 +1,102 @@
> +/*
> + * Faraday A360 PMU
> + *
> + * Copyright (c) 2012 Faraday Technology
> + * Written by Dante Su<address@hidden>
> + *
> + * This code is licensed under GNU GPL v2+
> + */
> +
> +#include <hw/hw.h>
> +#include <hw/sysbus.h>
> +#include <hw/devices.h>
> +#include <ui/console.h>
> +#include <qemu/timer.h>
> +#include <sysemu/sysemu.h>
> +
> +#include "faraday.h"
> +
> +#define REG_PDLLCR      0x30   /* PLL/DLL control register */
> +
> +#define TYPE_A360PMU    "a360.pmu"
> +
> +typedef struct A360PMUState {
> +    SysBusDevice busdev;
> +    MemoryRegion iomem;
> +} A360PMUState;
> +
> +#define A360PMU(obj) \
> +    OBJECT_CHECK(A360PMUState, obj, TYPE_A360PMU)
> +
> +static uint64_t
> +a360pmu_mem_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    uint64_t ret = 0;
> +
> +    switch (addr) {
> +    case REG_PDLLCR:
> +        ret = 27 << 3;  /* PLL = 27 */
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +static void
> +a360pmu_mem_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +}
> +

So, how does this memory behave on real hardware? Is it actually read as
zero (RZ/WI) at every offset except
at clock distribution control register? If not, I think you should model
it accordingly, by allocating a chunk of memory
0x1000 bytes length.


> +static const MemoryRegionOps a360pmu_mem_ops = {
> +    .read  = a360pmu_mem_read,
> +    .write = a360pmu_mem_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static int a360pmu_init(SysBusDevice *dev)
> +{
> +    A360PMUState *s = A360PMU(FROM_SYSBUS(A360PMUState, dev));

FROM_SYSBUS seems unnecessary here, you're casting to (Object *) in A360PMU
  anyway.


> +
> +    memory_region_init_io(&s->iomem,
> +                          &a360pmu_mem_ops,
> +                          s,
> +                          TYPE_A360PMU,
> +                          0x1000);
> +    sysbus_init_mmio(dev, &s->iomem);
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_a360pmu = {
> +    .name = TYPE_A360PMU,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_END_OF_LIST(),
> +    }
> +};
> +
> +static void a360pmu_class_init(ObjectClass *klass, void *data)
> +{
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    k->init   = a360pmu_init;
> +    dc->desc  = TYPE_A360PMU;
> +    dc->vmsd  = &vmstate_a360pmu;
> +    dc->no_user = 1;
> +}
> +
> +static const TypeInfo a360pmu_info = {
> +    .name          = TYPE_A360PMU,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(A360PMUState),
> +    .class_init    = a360pmu_class_init,
> +};
> +
> +static void a360pmu_register_types(void)
> +{
> +    type_register_static(&a360pmu_info);
> +}
> +
> +type_init(a360pmu_register_types)