Hi Kuo Jung,
On Thu, Mar 28, 2013 at 1:24 PM, Kuo-Jung Su <dant...@gmail.com> wrote:
> 2013/3/28 Peter Crosthwaite <peter.crosthwa...@xilinx.com>:
>> Hi Kuo-Jung,
>>
>> On Mon, Mar 25, 2013 at 10:09 PM, Kuo-Jung Su <dant...@gmail.com> wrote:
>>> From: Kuo-Jung Su <dant...@faraday-tech.com>
>>>
>>> The FTDDRII030 is a DDRII SDRAM controller which is responsible for
>>> SDRAM initialization.
>>>
>>> Signed-off-by: Kuo-Jung Su <dant...@faraday-tech.com>
>>> ---
>>> 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 <dant...@faraday-tech.com>
>>> + *
>>> + * 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 access@%#" 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 access@%#" 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
>
