Re: [Qemu-devel] [PATCH v1 11/12] hw/arm: versal: Add a model of Xilinx Versal SoC

[Edgar E. Iglesias]

On Mon, Oct 08, 2018 at 02:19:09PM +0100, Peter Maydell wrote:
> On 3 October 2018 at 16:07, Edgar E. Iglesias <address@hidden> wrote:
> > From: "Edgar E. Iglesias" <address@hidden>
> >
> > Add a model of Xilinx Versal SoC.
> >
> > Signed-off-by: Edgar E. Iglesias <address@hidden>
> > ---
> >  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