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

[Peter Crosthwaite]

Hi Kuo Jung,

On Thu, Mar 28, 2013 at 1:24 PM, Kuo-Jung Su <address@hidden> wrote:
> 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 new RAM region would have no awareness of its base address. It is
just a remappable container. The SoC can still do the remapping part.
This device model is then oblivous to the fact that the SoC and other
devices are remapping its base address.

> 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 *.
>

calling sysbus_mmio_get_region() on the SoC level should do the trick.
If you init the container region as proposed, this function can be
used by the SoC to get a handle to it.

I think we are on similar timezones (+10:00 here) if you want to try
and clarify on the IRC sometime today as well.

Regards,
Peter

> 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
>