qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v9 05/24] hw/arm: add FTDDRII030 DDRII controlle


From: Kuo-Jung Su
Subject: Re: [Qemu-devel] [PATCH v9 05/24] hw/arm: add FTDDRII030 DDRII controller support
Date: Thu, 28 Mar 2013 11:24:02 +0800

2013/3/28 Peter Crosthwaite <address@hidden>:
> Hi Kuo-Jung,
>
> On Mon, Mar 25, 2013 at 10:09 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.
>>
>> Signed-off-by: Kuo-Jung Su <address@hidden>
>> ---
>>  hw/arm/Makefile.objs    |    2 +-
>>  hw/arm/ftplat_a369soc.c |    8 ++
>>  hw/ftddrii030.c         |  192 
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 201 insertions(+), 1 deletion(-)
>>  create mode 100644 hw/ftddrii030.c
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index b2fa20f..e774962 100644
>> --- a/hw/arm/Makefile.objs
>> +++ b/hw/arm/Makefile.objs
>> @@ -24,7 +24,7 @@ obj-y += framebuffer.o
>>  obj-y += strongarm.o
>>  obj-y += imx_serial.o imx_ccm.o imx_timer.o imx_avic.o
>>  obj-$(CONFIG_KVM) += kvm/arm_gic.o
>> -obj-y += ftintc020.o ftahbc020.o
>> +obj-y += ftintc020.o ftahbc020.o ftddrii030.o
>>
>>  obj-y := $(addprefix ../,$(obj-y))
>>
>> diff --git a/hw/arm/ftplat_a369soc.c b/hw/arm/ftplat_a369soc.c
>> index 7f222cb..b2da582 100644
>> --- a/hw/arm/ftplat_a369soc.c
>> +++ b/hw/arm/ftplat_a369soc.c
>> @@ -129,6 +129,14 @@ static void a369soc_chip_init(FaradaySoCState *s)
>>          fprintf(stderr, "a369soc: Unable to set soc link for FTAHBC020\n");
>>          abort();
>>      }
>> +
>> +    /* ftddrii030 */
>> +    ds = sysbus_create_simple("ftddrii030", 0x93100000, NULL);
>> +    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/ftddrii030.c b/hw/ftddrii030.c
>> new file mode 100644
>> index 0000000..158db1f
>> --- /dev/null
>> +++ b/hw/ftddrii030.c
>> @@ -0,0 +1,192 @@
>> +/*
>> + * 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 "hw/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)  /* initialization finished */
>> +#define MSR_CMD_MRS         BIT(0)  /* start MRS command (init. seq.) */
>> +
>> +#define CFG_REGSIZE         (REG_REVR / 4)
>> +
>> +#define TYPE_FTDDRII030     "ftddrii030"
>> +
>> +typedef struct Ftddrii030State {
>> +    /*< private >*/
>> +    SysBusDevice parent;
>> +
>> +    /*< public >*/
>> +    MemoryRegion iomem;
>> +
>> +    FaradaySoCState *soc;
>> +
>
> This new implementation still has many of the same encapsulation
> issues discussed in v8. If you go with the suggestion myself and Peter
> came up with you could make this device completely self contained - it
> should have no awareness that it is part of the Faraday SoC. If you
> model as a second MemoryRegion you can push all the Faraday specific
> foo up to the SoC. Interdiff would go something like this:
>
> -    FaradaySoCState *soc;
> +    MemoryRegion ram;
>

Sorry, but I think it might not work in that way.
Because there is an AHB remap function is Faraday SoC,
which would alter the base address of both ROM and RAM.
So the ram instance should never be a local variable to FTDDRII030 only.

The first thing solution spring up in my mind is to add a QOM link in
FTDDRII030, and then pass the ram instance as a QOM link from device model.
However it turns out that it's also not possible, since the ram
instance (MemoryRegion) is not a Object *.

So I turn out to implement both the 'AHB remap' and 'RAM
initialization' as common routines
to Faraday SoC, and also create a remappable memory region for both ROM and RAM.

>> +    /* 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);
>> +    FaradaySoCState *soc = s->soc;
>> +    uint64_t ret = 0;
>> +
>> +    if (soc->ram_visible) {
>> +        DDR_REG32(s, REG_MSR) |= MSR_INIT_OK;
>> +    } else {
>> +        DDR_REG32(s, REG_MSR) &= ~MSR_INIT_OK;
>> +    }
>> +
>> +    switch (addr) {
>> +    case REG_MCR ... REG_REVR - 4:
>> +        ret = DDR_REG32(s, addr);
>> +        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);
>> +    FaradaySoCState *soc = s->soc;
>> +
>> +    switch (addr) {
>> +    case REG_MCR:
>> +        DDR_REG32(s, addr) = (uint32_t)val & 0xffff;
>> +        break;
>> +    case REG_MSR:
>> +        if (!soc->ram_visible && (val & MSR_CMD_MRS)) {
>> +            val &= ~MSR_CMD_MRS;
>> +            faraday_soc_ram_setup(soc, true);
>
> This function (added earlier in this series) then becomes local to
> this device model and the memory_region_add_subregion() happens with
> the new ram Memory region instead. AFAICT that function only requires
> the ram size which strikes me as something that should be a QOM
> property to the device.
>

Please see the comments above.

>> +        }
>> +        DDR_REG32(s, addr) = (uint32_t)val;
>> +        break;
>> +    /* SDRAM Timing, ECC ...etc. */
>> +    case REG_MSR + 4 ... REG_REVR - 4:
>> +        DDR_REG32(s, addr) = (uint32_t)val;
>> +        break;
>> +    case REG_REVR:
>> +        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();
>> +    }
>> +
>> +    faraday_soc_ram_setup(s->soc, false);
>> +    memset(s->regs, 0, sizeof(s->regs));
>> +}
>> +
>> +static void ftddrii030_realize(DeviceState *dev, Error **errp)
>> +{
>> +    Ftddrii030State *s = FTDDRII030(dev);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +
>> +    memory_region_init_io(&s->iomem,
>> +                          &mmio_ops,
>> +                          s,
>> +                          TYPE_FTDDRII030,
>> +                          0x1000);
>> +    sysbus_init_mmio(sbd, &s->iomem);
>
> Init the new ram memory region here.
>

Please see the comments above.

> I could use a second opinion on all this QOM stuff though - you are on
> the bleeding edge with the QOM SoC stuff. I suggest giving it some
> list time for others to reply before a remake.
>

Got it, thanks.

> Regards,
> Peter
>
>> +}
>> +
>> +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_instance_init(Object *obj)
>> +{
>> +    Ftddrii030State *s = FTDDRII030(obj);
>> +
>> +    object_property_add_link(obj,
>> +                             "soc",
>> +                             TYPE_FARADAY_SOC,
>> +                             (Object **) &s->soc,
>> +                             NULL);
>> +}
>> +
>> +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),
>> +    .instance_init = ftddrii030_instance_init,
>> +    .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
>>
>>



--
Best wishes,
Kuo-Jung Su



reply via email to

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