Hi Philippe,
> From: Jamin Lin
> Sent: Monday, February 3, 2025 3:29 PM
> To: Philippe Mathieu-Daudé <phi...@linaro.org>; Cédric Le Goater
> <c...@kaod.org>; Peter Maydell <peter.mayd...@linaro.org>; Andrew Jeffery
> <and...@codeconstruct.com.au>; Joel Stanley <j...@jms.id.au>; Alistair
> Francis <alist...@alistair23.me>; Cleber Rosa <cr...@redhat.com>; Wainer
> dos Santos Moschetta <waine...@redhat.com>; Beraldo Leal
> <bl...@redhat.com>; open list:ASPEED BMCs <qemu-...@nongnu.org>; open
> list:All patches CC here <qemu-devel@nongnu.org>; Jinjie Ruan
> <ruanjin...@huawei.com>
> Cc: Troy Lee <troy_...@aspeedtech.com>; Yunlin Tang
> <yunlin.t...@aspeedtech.com>
> Subject: RE: [PATCH v5 13/17] aspeed/soc: Add AST2700 support
>
> Hi Philippe,
>
> > From: Philippe Mathieu-Daudé <phi...@linaro.org>
> > Sent: Thursday, January 30, 2025 11:14 PM
> > To: Jamin Lin <jamin_...@aspeedtech.com>; Cédric Le Goater
> > <c...@kaod.org>; Peter Maydell <peter.mayd...@linaro.org>; Andrew
> > Jeffery <and...@codeconstruct.com.au>; Joel Stanley <j...@jms.id.au>;
> > Alistair Francis <alist...@alistair23.me>; Cleber Rosa
> > <cr...@redhat.com>; Wainer dos Santos Moschetta
> <waine...@redhat.com>;
> > Beraldo Leal <bl...@redhat.com>; open list:ASPEED BMCs
> > <qemu-...@nongnu.org>; open list:All patches CC here
> > <qemu-devel@nongnu.org>; Jinjie Ruan <ruanjin...@huawei.com>
> > Cc: Troy Lee <troy_...@aspeedtech.com>; Yunlin Tang
> > <yunlin.t...@aspeedtech.com>
> > Subject: Re: [PATCH v5 13/17] aspeed/soc: Add AST2700 support
> >
> > Hi Jamin,
> >
> > On 4/6/24 07:44, Jamin Lin wrote:
> > > Initial definitions for a simple machine using an AST2700 SOC
> > > (Cortex-a35
> > CPU).
> > >
> > > AST2700 SOC and its interrupt controller are too complex to handle
> > > in the common Aspeed SoC framework. We introduce a new ast2700 class
> > > with instance_init and realize handlers.
> > >
> > > AST2700 is a 64 bits quad core cpus and support 8 watchdog.
> > > Update maximum ASPEED_CPUS_NUM to 4 and ASPEED_WDTS_NUM to 8.
> > > In addition, update AspeedSocState to support scuio, sli, sliio and intc.
> > >
> > > Add TYPE_ASPEED27X0_SOC machine type.
> > >
> > > The SDMC controller is unlocked at SPL stage.
> > > At present, only supports to emulate booting start from u-boot stage.
> > > Set SDMC controller unlocked by default.
> > >
> > > In INTC, each interrupt of INT 128 to INT 136 combines 32 interrupts.
> > > It connect GICINT IRQ GPIO-OUTPUT pins to GIC device with irq 128 to 136.
> > > And, if a device irq is 128 to 136, its irq GPIO-OUTPUT pin is
> > > connected to GICINT or-gates instead of GIC device.
> > >
> > > Signed-off-by: Troy Lee <troy_...@aspeedtech.com>
> > > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
> > > ---
> > > hw/arm/aspeed_ast27x0.c | 563
> > ++++++++++++++++++++++++++++++++++++
> > > hw/arm/meson.build | 1 +
> > > include/hw/arm/aspeed_soc.h | 28 +-
> > > 3 files changed, 590 insertions(+), 2 deletions(-)
> > > create mode 100644 hw/arm/aspeed_ast27x0.c
> >
> >
> > > +static bool aspeed_soc_ast2700_gic_realize(DeviceState *dev, Error
> > > +**errp) {
> > > + 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;
> > > +
> > > + 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(&a->cpu[i]);
> > > + 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));
> >
> > Your patch was merged around the same time Jinjie added NMI support
> > (see commit b36a32ead1 "target/arm: Add support for Non-maskable
> Interrupt").
> >
> > Should we add them now?
> >
>
> After I read this commit, https://github.com/qemu/qemu/commit/b36a32ead,
> it seems that it improves the handling of ARM_CPU_VIRQ and
> ARM_CPU_VFIQ.
> I tested the changes by removing the following lines and worked for AST2700.
>
> 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));
>
> Are you suggesting that these lines should be removed?
>
> Thanks-Jamin
>
Sorry, I misunderstood your question.
I referred to this line,
https://github.com/qemu/qemu/blob/master/hw/arm/virt.c#L907, in QEMU's virt.c
to add NMI support.
Are you suggesting adding the following lines to enable NMI support?
It worked as well.
```
sysbus_connect_irq(gicbusdev, i + 4 * sc->num_cpus,
qdev_get_gpio_in(cpudev, ARM_CPU_NMI));
sysbus_connect_irq(gicbusdev, i + 5 * sc->num_cpus,
qdev_get_gpio_in(cpudev, ARM_CPU_VINMI));
```
Thanks-Jamin
> > > + }
> > > +
> > > + return true;
> > > +}
