From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59147) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1g9VRc-00085V-W8 for qemu-devel@nongnu.org; Mon, 08 Oct 2018 09:19:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1g9VRb-000320-JK for qemu-devel@nongnu.org; Mon, 08 Oct 2018 09:19:32 -0400 Received: from mail-ot1-x341.google.com ([2607:f8b0:4864:20::341]:35600) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1g9VRb-00030E-6U for qemu-devel@nongnu.org; Mon, 08 Oct 2018 09:19:31 -0400 Received: by mail-ot1-x341.google.com with SMTP id j9-v6so19576947otl.2 for ; Mon, 08 Oct 2018 06:19:30 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1538579266-8389-12-git-send-email-edgar.iglesias@gmail.com> References: <1538579266-8389-1-git-send-email-edgar.iglesias@gmail.com> <1538579266-8389-12-git-send-email-edgar.iglesias@gmail.com> From: Peter Maydell Date: Mon, 8 Oct 2018 14:19:09 +0100 Message-ID: Content-Type: text/plain; charset="UTF-8" Subject: Re: [Qemu-devel] [PATCH v1 11/12] hw/arm: versal: Add a model of Xilinx Versal SoC List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Edgar E. Iglesias" Cc: QEMU Developers , qemu-arm , Richard Henderson , KONRAD Frederic , Alistair Francis , Francisco Iglesias , figlesia@xilinx.com, Stefano Stabellini , Sai Pavan Boddu , Edgar Iglesias On 3 October 2018 at 16:07, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" > > Add a model of Xilinx Versal SoC. > > Signed-off-by: Edgar E. Iglesias > --- > default-configs/aarch64-softmmu.mak | 1 + > hw/arm/Makefile.objs | 1 + > hw/arm/xlnx-versal.c | 339 ++++++++++++++++++++++++++++++++++++ > include/hw/arm/xlnx-versal.h | 122 +++++++++++++ > 4 files changed, 463 insertions(+) > create mode 100644 hw/arm/xlnx-versal.c > create mode 100644 include/hw/arm/xlnx-versal.h > > +#define XLNX_VERSAL_ACPU_TYPE "cortex-a72" "-" TYPE_ARM_CPU ARM_CPU_TYPE_NAME("cortex-a72") is preferable to hand-assembling the type name like this. > +#define GEM_REVISION 0x40070106 > + > + for (i = 0; i < nr_apu_cpus; i++) { > + DeviceState *cpudev = DEVICE(s->fpd.apu.cpu[i]); > + int ppibase = XLNX_VERSAL_NR_IRQS + i * GIC_INTERNAL + GIC_NR_SGIS; > + qemu_irq maint_irq; > + int ti; > + /* Mapping from the output timer irq lines from the CPU to the > + * GIC PPI inputs we use for the virt board. > + */ This isn't the virt board :-) -- cut-n-pasted comment ? > + const int timer_irq[] = { > + [GTIMER_PHYS] = VERSAL_TIMER_NS_EL1_IRQ, > + [GTIMER_VIRT] = VERSAL_TIMER_VIRT_IRQ, > + [GTIMER_HYP] = VERSAL_TIMER_NS_EL2_IRQ, > + [GTIMER_SEC] = VERSAL_TIMER_S_EL1_IRQ, > + }; > + > + for (ti = 0; ti < ARRAY_SIZE(timer_irq); ti++) { > + qdev_connect_gpio_out(cpudev, ti, > + qdev_get_gpio_in(gicdev, > + ppibase + timer_irq[ti])); > + } > + maint_irq = qdev_get_gpio_in(gicdev, > + ppibase + VERSAL_GIC_MAINT_IRQ); > + qdev_connect_gpio_out_named(cpudev, "gicv3-maintenance-interrupt", > + 0, maint_irq); > + sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ)); > + sysbus_connect_irq(gicbusdev, i + nr_apu_cpus, > + qdev_get_gpio_in(cpudev, ARM_CPU_FIQ)); > + sysbus_connect_irq(gicbusdev, i + 2 * nr_apu_cpus, > + qdev_get_gpio_in(cpudev, ARM_CPU_VIRQ)); > + sysbus_connect_irq(gicbusdev, i + 3 * nr_apu_cpus, > + qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ)); > + } > + > + for (i = 0; i < XLNX_VERSAL_NR_IRQS; i++) { > + pic[i] = qdev_get_gpio_in(gicdev, i); > + } > +/* This takes the board allocated linear DDR memory and creates aliases > + * for each split DDR range/apperture on the Versal address map. "aperture" > +static void versal_realize(DeviceState *dev, Error **errp) > +{ > + Versal *s = XLNX_VERSAL(dev); > + qemu_irq pic[XLNX_VERSAL_NR_IRQS]; > + > + versal_create_apu_cpus(s, errp); > + versal_create_apu_gic(s, pic, errp); > + versal_create_uarts(s, pic); > + versal_create_gems(s, pic); > + versal_map_ddr(s); > + versal_unimp(s); > + > + /* Create the OCM. */ > + memory_region_init_ram(&s->lpd.mr_ocm, OBJECT(s), "ocm", > + MM_OCM_SIZE, &error_fatal); What's an OCM? Is it really memory, or is this a stub for something? > + > + memory_region_add_subregion_overlap(&s->mr_ps, MM_OCM, &s->lpd.mr_ocm, 0); > + memory_region_add_subregion_overlap(&s->fpd.apu.mr, 0, &s->mr_ps, 0); > +} > + > +static void versal_init(Object *obj) > +{ > + Versal *s = XLNX_VERSAL(obj); > + > + memory_region_init(&s->fpd.apu.mr, obj, "mr-apu", UINT64_MAX); > + memory_region_init(&s->mr_ps, obj, "mr-ps-switch", UINT64_MAX); > +} > + > +static const VMStateDescription versal_vmstate = { > + .name = "xlnx-ve", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + /* FIXME. */ Stray FIXME comment -- I think the answer may be "the SoC object has no state of its own so needs neither a vmsd nor a reset method" ? (If so and if you drop them, do put a comment in the class init about why they're not provided. Some day we may have a mechanism for a device to explicitly say "I need no vmstate" so we can assert if none is provided.) > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static Property versal_properties[] = { > + DEFINE_PROP_LINK("ddr", Versal, cfg.mr_ddr, TYPE_MEMORY_REGION, > + MemoryRegion *), > + DEFINE_PROP_UINT32("psci-conduit", Versal, cfg.psci_conduit, 0), > + DEFINE_PROP_END_OF_LIST() > +}; > + > +static void versal_reset(DeviceState *dev) > +{ > +} > + > +static void versal_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->realize = versal_realize; > + dc->vmsd = &versal_vmstate; > + dc->props = versal_properties; > + dc->reset = versal_reset; > +} Looks good otherwise. thanks -- PMM