On Mon, Oct 08, 2018 at 02:19:09PM +0100, Peter Maydell wrote: > On 3 October 2018 at 16:07, Edgar E. Iglesias <edgar.igles...@gmail.com> > wrote: > > From: "Edgar E. Iglesias" <edgar.igles...@xilinx.com> > > > > Add a model of Xilinx Versal SoC. > > > > Signed-off-by: Edgar E. Iglesias <edgar.igles...@xilinx.com> > > --- > > 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.
Fixed for v2. > > > +#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 ? Fixed for v2. > > > + 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" Fixed > > > > > +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? I've changed the comment to spell out that it's an On Chip Memory. > > > + > > + 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.) Thanks, I'll do that. > > > + 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, Edgar