Re: [Qemu-devel] [PATCH v8 05/24] hw/arm: add FTDDRII030 DDRII controller support

[Peter Crosthwaite]

Hi Kuo-Jung,

On Fri, Mar 15, 2013 at 11:13 PM, Kuo-Jung Su <address@hidden> wrote:
> From: Kuo-Jung Su <address@hidden>
>
> The FTDDRII030 is a DDRII SDRAM controller which is responsible for
> SDRAM initialization.
> In QEMU we emulate only the SDRAM enable function.
>
> Signed-off-by: Kuo-Jung Su <address@hidden>
> ---
>  hw/arm/Makefile.objs      |    1 +
>  hw/arm/faraday_a369_soc.c |    9 +++
>  hw/arm/ftddrii030.c       |  183 
> +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 193 insertions(+)
>  create mode 100644 hw/arm/ftddrii030.c
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index af36b01..0bbf838 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -39,3 +39,4 @@ obj-y += faraday_a369.o faraday_a369_soc.o 
> faraday_a369_scu.o \
>              faraday_a369_kpd.o
>  obj-y += ftintc020.o
>  obj-y += ftahbc020.o
> +obj-y += ftddrii030.o
> diff --git a/hw/arm/faraday_a369_soc.c b/hw/arm/faraday_a369_soc.c
> index 01b4395..e8a63bb 100644
> --- a/hw/arm/faraday_a369_soc.c
> +++ b/hw/arm/faraday_a369_soc.c
> @@ -158,6 +158,15 @@ a369soc_device_init(FaradaySoCState *s)
>          fprintf(stderr, "a369soc: Unable to set soc link for FTAHBC020\n");
>          abort();
>      }
> +
> +    /* ftddrii030 */
> +    ds = sysbus_create_simple("ftddrii030", 0x93100000, NULL);
> +    s->ddrc = ds;
> +    object_property_set_link(OBJECT(ds), OBJECT(s), "soc", &local_errp);
> +    if (local_errp) {
> +        fprintf(stderr, "a369soc: Unable to set soc link for FTDDRII030\n");
> +        abort();
> +    }
>  }
>
>  static void a369soc_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/arm/ftddrii030.c b/hw/arm/ftddrii030.c
> new file mode 100644
> index 0000000..90a5842
> --- /dev/null
> +++ b/hw/arm/ftddrii030.c
> @@ -0,0 +1,183 @@
> +/*
> + * Faraday DDRII controller
> + *
> + * 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 "sysemu/sysemu.h"
> +
> +#include "faraday.h"
> +
> +#define REG_MCR             0x00    /* memory configuration register */
> +#define REG_MSR             0x04    /* memory status register */
> +#define REG_REVR            0x50    /* revision register */
> +
> +#define MSR_INIT_OK         BIT(8)  /* DDR2 initial is completed */
> +#define MSR_CMD_MRS         BIT(0)  /* start MRS command */
> +
> +#define CFG_REGSIZE         (0x50 / 4)
> +
> +#define TYPE_FTDDRII030     "ftddrii030"
> +
> +typedef struct Ftddrii030State {
> +    SysBusDevice busdev;
> +    MemoryRegion iomem;
> +
> +    FaradaySoCState *soc;
> +    /* HW register cache */
> +    uint32_t regs[CFG_REGSIZE];
> +} Ftddrii030State;
> +
> +#define FTDDRII030(obj) \
> +    OBJECT_CHECK(Ftddrii030State, obj, TYPE_FTDDRII030)
> +
> +#define DDR_REG32(s, off) \
> +    ((s)->regs[(off) / 4])
> +
> +static uint64_t
> +ftddrii030_mem_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    Ftddrii030State *s = FTDDRII030(opaque);
> +    uint64_t ret = 0;
> +
> +    if (s->soc->ddr_inited) {
> +        DDR_REG32(s, REG_MSR) |= MSR_INIT_OK;
> +    }
> +
> +    switch (addr) {
> +    case REG_MCR ... (CFG_REGSIZE - 1) * 4:
> +        ret = s->regs[addr / 4];
> +        break;
> +    case REG_REVR:
> +        ret = 0x100;    /* rev. = 0.1.0 */
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "ftddrii030: undefined memory address@hidden" HWADDR_PRIx "\n", 
> addr);
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +static void
> +ftddrii030_mem_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +    Ftddrii030State *s = FTDDRII030(opaque);
> +
> +    switch (addr) {
> +    case REG_MCR:
> +        DDR_REG32(s, REG_MCR) = (uint32_t)val & 0xffff;
> +        break;
> +    case REG_MSR:
> +        val = (val & 0x3f) | (DDR_REG32(s, REG_MSR) & MSR_INIT_OK);
> +        if (!s->soc->ddr_inited && (val & MSR_CMD_MRS)) {
> +            val &= ~MSR_CMD_MRS;
> +            val |= MSR_INIT_OK;
> +            memory_region_add_subregion(s->soc->as,
> +                                        s->soc->ram_base,
> +                                        s->soc->ram);

I feel like this is overstepping the bounds of the device. Its
modifying the internals of the parent device (the SoC itself). AFAICT,
this device does not need awareness of where the RAM is to live in the
address map, thats the responsibility of the machine model. It might
be cleaner to model the actual RAM as a second sysbus memory region
then leave it up the machine model to decide where in the address map
it should live. This device just adds/removes the ram from the second
region without knowing where it lives and the machine model maps the
RAM to its actual location. Keeps .as .ram_base and .ram private to
the SoC device.

> +            s->soc->ddr_inited = true;

I'm still trying to figure out the physical analogue of this. Is there
a genuine hardware linkage from the DDR controller to other devices
that says "hey i'm inited" or is this faking firmware activity? In the
former case, this ddr_inited should be a GPIO from DDR controller to
whatever devices care. In the latter case, its trickier, and we should
discuss bootloader based solutions to get your tiny little bit of
firmware in, without having to model non-existent hardware.

Regards,
Peter

> +        }
> +        DDR_REG32(s, REG_MSR) = (uint32_t)val;
> +        break;
> +    case 0x08 ... (CFG_REGSIZE - 1) * 4: /* DDRII Timing, ECC ...etc. */
> +        s->regs[addr / 4] = (uint32_t)val;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "ftddrii030: undefined memory address@hidden" HWADDR_PRIx "\n", 
> addr);
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps mmio_ops = {
> +    .read  = ftddrii030_mem_read,
> +    .write = ftddrii030_mem_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    }
> +};
> +
> +static void ftddrii030_reset(DeviceState *ds)
> +{
> +    Ftddrii030State *s = FTDDRII030(SYS_BUS_DEVICE(ds));
> +    Error *local_errp = NULL;
> +
> +    s->soc = FARADAY_SOC(object_property_get_link(OBJECT(s),
> +                                                  "soc",
> +                                                  &local_errp));
> +    if (local_errp) {
> +        fprintf(stderr, "ftahbc020: Unable to get soc link\n");
> +        abort();
> +    }
> +
> +    if (s->soc->ddr_inited && !s->soc->bi) {
> +        memory_region_del_subregion(s->soc->as, s->soc->ram);
> +        s->soc->ddr_inited = false;
> +    }
> +
> +    memset(s->regs, 0, sizeof(s->regs));
> +}
> +
> +static void ftddrii030_realize(DeviceState *dev, Error **errp)
> +{
> +    Ftddrii030State *s = FTDDRII030(dev);
> +
> +    memory_region_init_io(&s->iomem,
> +                          &mmio_ops,
> +                          s,
> +                          TYPE_FTDDRII030,
> +                          0x1000);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
> +
> +    object_property_add_link(OBJECT(dev),
> +                             "soc",
> +                             TYPE_FARADAY_SOC,
> +                             (Object **) &s->soc,
> +                             errp);
> +}
> +
> +static const VMStateDescription vmstate_ftddrii030 = {
> +    .name = TYPE_FTDDRII030,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(regs, Ftddrii030State, CFG_REGSIZE),
> +        VMSTATE_END_OF_LIST(),
> +    }
> +};
> +
> +static void ftddrii030_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->desc  = TYPE_FTDDRII030;
> +    dc->vmsd  = &vmstate_ftddrii030;
> +    dc->reset = ftddrii030_reset;
> +    dc->realize = ftddrii030_realize;
> +    dc->no_user = 1;
> +}
> +
> +static const TypeInfo ftddrii030_info = {
> +    .name          = TYPE_FTDDRII030,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(Ftddrii030State),
> +    .class_init    = ftddrii030_class_init,
> +};
> +
> +static void ftddrii030_register_types(void)
> +{
> +    type_register_static(&ftddrii030_info);
> +}
> +
> +type_init(ftddrii030_register_types)
> --
> 1.7.9.5
>
>