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

[Peter Crosthwaite]

On Mon, Mar 18, 2013 at 11:12 AM, Kuo-Jung Su <address@hidden> wrote:
> 2013/3/16 Peter Crosthwaite <address@hidden>:
>> 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.
>>
>
> Thanks for the comments,
> I'll try to implement a cleaner model in the way suggested in the
> above comments.
>
>>> +            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.
>>
>
> Thanks for the comments.
> It's the 1st one, it's used to inform the FTAHBC020 of the presence of
> DRAM in QEMU model. And thus it could be replaced by a GPIO
> implementation.

But what's the underlying transport mechanism for this information
this in silicon? Is there a wire from the DDR controller to the AHB
controller? Or is it some sort of other linkage? Do you have some
data-sheets for these two you could quickly link me? Might me able to
give a better suggestion with a quick scan of specs.

Regards,
Peter