From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51383) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UL5OO-0003ke-FG for qemu-devel@nongnu.org; Thu, 28 Mar 2013 01:28:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UL5OL-0002VJ-8l for qemu-devel@nongnu.org; Thu, 28 Mar 2013 01:28:52 -0400 Received: from mail-ia0-x22e.google.com ([2607:f8b0:4001:c02::22e]:35065) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UL5OL-0002V8-1Q for qemu-devel@nongnu.org; Thu, 28 Mar 2013 01:28:49 -0400 Received: by mail-ia0-f174.google.com with SMTP id b35so7852413iac.19 for ; Wed, 27 Mar 2013 22:28:48 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1364213400-10266-1-git-send-email-dantesu@gmail.com> <1364213400-10266-6-git-send-email-dantesu@gmail.com> Date: Thu, 28 Mar 2013 13:28:48 +0800 Message-ID: From: Kuo-Jung Su Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v9 05/24] hw/arm: add FTDDRII030 DDRII controller support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , Igor Mitsyanko , qemu-devel@nongnu.org, Blue Swirl , Paul Brook , Kuo-Jung Su , Andreas , fred konrad 2013/3/28 Peter Crosthwaite : > Hi Kuo Jung, > > On Thu, Mar 28, 2013 at 1:24 PM, Kuo-Jung Su wrote: >> 2013/3/28 Peter Crosthwaite : >>> Hi Kuo-Jung, >>> >>> On Mon, Mar 25, 2013 at 10:09 PM, Kuo-Jung Su wrote: >>>> From: Kuo-Jung Su >>>> >>>> The FTDDRII030 is a DDRII SDRAM controller which is responsible for >>>> SDRAM initialization. >>>> >>>> Signed-off-by: Kuo-Jung Su >>>> --- >>>> 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 >>>> + * >>>> + * 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 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 >> -- Best wishes, Kuo-Jung Su