Re: [Qemu-devel] [PATCH v1 11/12] hw/arm: versal: Add a model of Xilinx Versal SoC
On Mon, Oct 08, 2018 at 02:19:09PM +0100, Peter Maydell wrote:
> 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.
Fixed for v2.
>
> > +#define GEM_REVISION0x40070106
> > +
>
> > +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(>lpd.mr_ocm, OBJECT(s), "ocm",
> > + MM_OCM_SIZE, _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(>mr_ps, MM_OCM, >lpd.mr_ocm,
> > 0);
> > +memory_region_add_subregion_overlap(>fpd.apu.mr, 0, >mr_ps, 0);
> > +}
> > +
> > +static void versal_init(Object *obj)
> > +{
> > +Versal *s = XLNX_VERSAL(obj);
> > +
> > +memory_region_init(>fpd.apu.mr, obj, "mr-apu", UINT64_MAX);
> > +memory_region_init(>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,
Re: [Qemu-devel] [PATCH v1 11/12] hw/arm: versal: Add a model of Xilinx Versal SoC
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_REVISION0x40070106
> +
> +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(>lpd.mr_ocm, OBJECT(s), "ocm",
> + MM_OCM_SIZE, _fatal);
What's an OCM? Is it really memory, or is this a stub for something?
> +
> +memory_region_add_subregion_overlap(>mr_ps, MM_OCM, >lpd.mr_ocm,
> 0);
> +memory_region_add_subregion_overlap(>fpd.apu.mr, 0, >mr_ps, 0);
> +}
> +
> +static void versal_init(Object *obj)
> +{
> +Versal *s = XLNX_VERSAL(obj);
> +
> +memory_region_init(>fpd.apu.mr, obj, "mr-apu", UINT64_MAX);
> +memory_region_init(>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 = _vmstate;
> +dc->props = versal_properties;
> +dc->reset = versal_reset;
> +}
Looks good otherwise.
thanks
-- PMM
Re: [Qemu-devel] [PATCH v1 11/12] hw/arm: versal: Add a model of Xilinx Versal SoC
On 03/10/2018 17: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
>
> diff --git a/default-configs/aarch64-softmmu.mak
> b/default-configs/aarch64-softmmu.mak
> index 6f790f0..4ea9add 100644
> --- a/default-configs/aarch64-softmmu.mak
> +++ b/default-configs/aarch64-softmmu.mak
> @@ -8,4 +8,5 @@ CONFIG_DDC=y
> CONFIG_DPCD=y
> CONFIG_XLNX_ZYNQMP=y
> CONFIG_XLNX_ZYNQMP_ARM=y
> +CONFIG_XLNX_VERSAL=y
> CONFIG_ARM_SMMUV3=y
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 5f88062..ec21d9b 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -26,6 +26,7 @@ obj-$(CONFIG_ALLWINNER_A10) += allwinner-a10.o cubieboard.o
> obj-$(CONFIG_RASPI) += bcm2835_peripherals.o bcm2836.o raspi.o
> obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
> obj-$(CONFIG_XLNX_ZYNQMP_ARM) += xlnx-zynqmp.o xlnx-zcu102.o
> +obj-$(CONFIG_XLNX_VERSAL) += xlnx-versal.o
> obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o imx25_pdk.o
> obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
> obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
> diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> new file mode 100644
> index 000..c12fc85
> --- /dev/null
> +++ b/hw/arm/xlnx-versal.c
> @@ -0,0 +1,339 @@
> +/*
> + * Xilinx Versal SoC model.
> + *
> + * Copyright (c) 2018 Xilinx Inc.
> + * Written by Edgar E. Iglesias
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or
> + * (at your option) any later version.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +#include "qemu/log.h"
> +#include "hw/sysbus.h"
> +#include "net/net.h"
> +#include "sysemu/sysemu.h"
> +#include "sysemu/kvm.h"
> +#include "hw/arm/arm.h"
> +#include "kvm_arm.h"
> +#include "hw/misc/unimp.h"
> +#include "hw/intc/arm_gicv3_common.h"
> +#include "hw/arm/xlnx-versal.h"
> +
> +#define XLNX_VERSAL_ACPU_TYPE "cortex-a72" "-" TYPE_ARM_CPU
> +#define GEM_REVISION0x40070106
> +
> +static void versal_create_apu_cpus(Versal *s, Error **errp)
> +{
> +int i;
> +
> +for (i = 0; i < ARRAY_SIZE(s->fpd.apu.cpu); i++) {
> +Object *obj;
> +char *name;
> +
> +obj = object_new(XLNX_VERSAL_ACPU_TYPE);
> +if (!obj) {
> +/* Secondary CPUs start in PSCI powered-down state */
> +error_setg(errp, "Unable to create apu.cpu[%d] of type %s",
> + i, XLNX_VERSAL_ACPU_TYPE);
> +return;
> +}
> +
> +name = g_strdup_printf("apu-cpu[%d]", i);
> +object_property_add_child(OBJECT(s), name, obj, _fatal);
> +g_free(name);
> +
> +object_property_set_int(obj, s->cfg.psci_conduit,
> +"psci-conduit", _abort);
> +if (i) {
> +object_property_set_bool(obj, true,
> + "start-powered-off", _abort);
> +}
> +
> +object_property_set_int(obj, ARRAY_SIZE(s->fpd.apu.cpu),
> +"core-count", _abort);
> +object_property_set_link(obj, OBJECT(>fpd.apu.mr), "memory",
> + _abort);
> +object_property_set_bool(obj, true, "realized", _fatal);
> +s->fpd.apu.cpu[i] = ARM_CPU(obj);
> +}
> +}
> +
> +static void versal_create_apu_gic(Versal *s, qemu_irq *pic, Error **errp)
> +{
> +static const uint64_t addrs[] = {
> +MM_GIC_APU_DIST_MAIN,
> +MM_GIC_APU_REDIST_0
> +};
> +SysBusDevice *gicbusdev;
> +DeviceState *gicdev;
> +int nr_apu_cpus = ARRAY_SIZE(s->fpd.apu.cpu);
> +int i;
> +
> +sysbus_init_child_obj(OBJECT(s), "apu-gic",
> + >fpd.apu.gic, sizeof(s->fpd.apu.gic),
> + gicv3_class_name());
> +gicbusdev = SYS_BUS_DEVICE(>fpd.apu.gic);
> +gicdev = DEVICE(>fpd.apu.gic);
> +qdev_prop_set_uint32(gicdev, "revision", 3);
> +qdev_prop_set_uint32(gicdev, "num-cpu", 2);
> +qdev_prop_set_uint32(gicdev, "num-irq", XLNX_VERSAL_NR_IRQS + 32);
> +qdev_prop_set_uint32(gicdev, "len-redist-region-count", 1);
> +qdev_prop_set_uint32(gicdev, "redist-region-count[0]", 2);
> +if (!kvm_irqchip_in_kernel()) {
> +qdev_prop_set_bit(gicdev, "has-security-extensions", true);
> +}
> +
> +object_property_set_bool(OBJECT(>fpd.apu.gic), true, "realized",
> errp);
> +
> +for (i = 0; i < ARRAY_SIZE(addrs); i++) {
> +MemoryRegion *mr;
> +
> +mr =
