Hi Cedric,
> From: Cédric Le Goater <c...@kaod.org>
> Subject: Re: [PATCH v4 12/16] aspeed/soc: Add AST2700 support
>
>
> Hello Jamin,
> > I refer to versal_create_apu_gic function,
> https://github.com/qemu/qemu/blob/master/hw/arm/xlnx-versal.c#L67
> > and updated aspeed_soc_ast2700_gic as following.
> > If you have any concerned about the new changes, please let me know.
> > Thanks-Jamin
> >
> > static bool aspeed_soc_ast2700_gic(DeviceState *dev, Error **errp)
>
> Please rename to aspeed_soc_ast2700_gic_realize()
Will fix in v5 patch
>
> > {
> > Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev);
> > AspeedSoCState *s = ASPEED_SOC(dev);
> > AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> > SysBusDevice *gicbusdev;
> > DeviceState *gicdev;
> > QList *redist_region_count;
> > int i;
> >
> > object_initialize_child(OBJECT(a), "ast2700-gic", &a->gic,
> > gicv3_class_name());
>
> and object_initialize_child() can be called in aspeed_soc_ast2700_init().
Will fix in v5 patch
My new changes as following,
static void aspeed_soc_ast2700_init(Object *obj) {
--
object_initialize_child(obj, "gic", &a->gic, gicv3_class_name());
--
}
>
> > gicbusdev = SYS_BUS_DEVICE(&a->gic);
> > gicdev = DEVICE(&a->gic);
> > qdev_prop_set_uint32(gicdev, "revision", 3);
> > qdev_prop_set_uint32(gicdev, "num-cpu", sc->num_cpus);
> > qdev_prop_set_uint32(gicdev, "num-irq", AST2700_MAX_IRQ);
> >
> > redist_region_count = qlist_new();
> > qlist_append_int(redist_region_count, sc->num_cpus);
> > qdev_prop_set_array(gicdev, "redist-region-count",
> redist_region_count);
> >
> > if (!sysbus_realize(gicbusdev, errp)) {
> > return false;
> > }
> > sysbus_mmio_map(gicbusdev, 0, sc->memmap[ASPEED_GIC_DIST]);
> > sysbus_mmio_map(gicbusdev, 1,
> sc->memmap[ASPEED_GIC_REDIST]);
> >
> > for (i = 0; i < sc->num_cpus; i++) {
> > DeviceState *cpudev = DEVICE(qemu_get_cpu(i));
>
> Could we avoid qemu_get_cpu() and use the cpu array of the SoC instead ?
Yes, I can change it and my new changes as following.
DeviceState *cpudev = DEVICE(&a->cpu[i]);
Will fix in v5 patch.
Thanks for review.
Jamin
>
> > int NUM_IRQS = 256, ARCH_GIC_MAINT_IRQ = 9,
> VIRTUAL_PMU_IRQ = 7;
> > int ppibase = NUM_IRQS + i * GIC_INTERNAL + GIC_NR_SGIS;
> >
> > const int timer_irq[] = {
> > [GTIMER_PHYS] = 14,
> > [GTIMER_VIRT] = 11,
> > [GTIMER_HYP] = 10,
> > [GTIMER_SEC] = 13,
> > };
> > int j;
> >
> > for (j = 0; j < ARRAY_SIZE(timer_irq); j++) {
> > qdev_connect_gpio_out(cpudev, j,
> > qdev_get_gpio_in(gicdev, ppibase + timer_irq[j]));
> > }
> >
> > qemu_irq irq = qdev_get_gpio_in(gicdev,
> > ppibase +
> ARCH_GIC_MAINT_IRQ);
> > qdev_connect_gpio_out_named(cpudev,
> "gicv3-maintenance-interrupt",
> > 0, irq);
> > qdev_connect_gpio_out_named(cpudev, "pmu-interrupt", 0,
> > qdev_get_gpio_in(gicdev, ppibase +
> VIRTUAL_PMU_IRQ));
> >
> > sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev,
> ARM_CPU_IRQ));
> > sysbus_connect_irq(gicbusdev, i + sc->num_cpus,
> > qdev_get_gpio_in(cpudev,
> ARM_CPU_FIQ));
> > sysbus_connect_irq(gicbusdev, i + 2 * sc->num_cpus,
> > qdev_get_gpio_in(cpudev,
> ARM_CPU_VIRQ));
> > sysbus_connect_irq(gicbusdev, i + 3 * sc->num_cpus,
> > qdev_get_gpio_in(cpudev,
> ARM_CPU_VFIQ));
> > }
> >
> > return true;
> > }
> >
> > struct Aspeed27x0SoCState {
> > AspeedSoCState parent;
> >
> > ARMCPU cpu[ASPEED_CPUS_NUM];
> > AspeedINTCState intc;
> > GICv3State gic;
> > };
> >
> > #define TYPE_ASPEED27X0_SOC "aspeed27x0-soc"
> > OBJECT_DECLARE_SIMPLE_TYPE(Aspeed27x0SoCState, ASPEED27X0_SOC)
>
> Thanks,
>
> C.
>
>
