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

[Kuo-Jung Su]

2013/3/28 Peter Crosthwaite <address@hidden>:
> 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.
>

It sounds like pretty good idea to me, I think I know what to do now.
Thanks

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

Thanks for the invitation, but the IRQ is banned in my company...

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



--
Best wishes,
Kuo-Jung Su