Hi Cedric,
> From: Cédric Le Goater <c...@kaod.org>
> Sent: Tuesday, February 4, 2025 3:35 PM
> To: Jamin Lin <jamin_...@aspeedtech.com>; Andrew Jeffery
> <and...@codeconstruct.com.au>; Peter Maydell <peter.mayd...@linaro.org>;
> Steven Lee <steven_...@aspeedtech.com>; Troy Lee <leet...@gmail.com>;
> Joel Stanley <j...@jms.id.au>; open list:ASPEED BMCs
> <qemu-...@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_...@aspeedtech.com>; Yunlin Tang
> <yunlin.t...@aspeedtech.com>
> Subject: Re: [PATCH v1 01/18] hw/intc/aspeed: Rename INTC to INTC0
>
> On 2/4/25 07:50, Jamin Lin wrote:
> > Hi Cedric, Andrew
> >
> >> From: Andrew Jeffery <and...@codeconstruct.com.au>
> >> Sent: Thursday, January 30, 2025 11:22 AM
> >> To: Cédric Le Goater <c...@kaod.org>; Jamin Lin
> >> <jamin_...@aspeedtech.com>; Peter Maydell <peter.mayd...@linaro.org>;
> >> Steven Lee <steven_...@aspeedtech.com>; Troy Lee
> <leet...@gmail.com>;
> >> Joel Stanley <j...@jms.id.au>; open list:ASPEED BMCs
> >> <qemu-...@nongnu.org>; open list:All patches CC here
> >> <qemu-devel@nongnu.org>
> >> Cc: Troy Lee <troy_...@aspeedtech.com>; Yunlin Tang
> >> <yunlin.t...@aspeedtech.com>
> >> Subject: Re: [PATCH v1 01/18] hw/intc/aspeed: Rename INTC to INTC0
> >>
> >> On Wed, 2025-01-29 at 18:03 +0100, Cédric Le Goater wrote:
> >>> On 1/21/25 08:04, Jamin Lin wrote:
> >>>> The design of the INTC has significant changes in the AST2700 A1.
> >>>> In the
> >>>> AST2700 A0, there was one INTC controller, whereas in the AST2700
> >>>> A1, there were two INTC controllers: INTC0 (CPU DIE) and INTC1 (I/O
> >>>> DIE).
> >>>>
> >>>> The previous INTC model only supported the AST2700 A0 and was
> >>>> implemented for the INTC0 (CPU DIE). To support the future INTC1
> >>>> (I/O DIE) model, rename INTC to INTC0.
> >>>
> >>>
> >>> Why not introduce definitions with _INTC_IO_ and leave alone the
> >>> current instead ? Do we expect to have more than 2 INTC controllers ?
> >>>
> >>
> >> There was similar discussion on the devicetree bindings for the SCU a
> >> while back:
> >>
> >> https://lore.kernel.org/all/94efc2d4ff280a112b869124fc9d7e35ac031596.
> >> cam
> >> e...@codeconstruct.com.au/
> >>
> >> Ryan didn't like deviating from their internal documentation :(
> >>
> >> Andrew
> >
> >
> > Thanks for your suggestion.
> >
> > Last year, Troy and I implemented the SCU(CPU Die) and SCU_IO(IO Die)
> models to support the AST2700.
> > https://github.com/qemu/qemu/blob/master/hw/misc/aspeed_scu.c#L1073
> > https://github.com/qemu/qemu/blob/master/hw/misc/aspeed_scu.c#L1080
> > > I am fine with keeping the INTC(CPU Die) naming and creating a new
> INTC_IO(IO Die) model to support the AST2700 A1.
>
> Good. I think this will reduce the changes and clarify the models.
>
> > I have a question regarding the INTC_IO model implementation:
> > Can I define separate "intc_io_class_init" and "intcio_class_realize"
> > functions
> for INTC_IO, similar to the SCU/SCU_IO models?
>
> Looks OK to me.
>
> > If yes, I think the patch “2 Support different memory region ops” can be
> omitted.
> > Additionally, I suggest that both INTC and INTC_IO have their own MMIO
> callback functions, as their register addresses are different.
>
> Do you mean the register offset in the MMIO aperture ? We try to avoid
> duplication unless the code becomes too complex.
I means both INTC_IO and INTC_CPU use the same offset but different register
definitions.
INTC0:
INTC0_10
INTC0_14
INTC1:
INTC1_10
INTC1_14
I will implement as following
static void aspeed_intc_register_types(void)
{
type_register_static(&aspeed_intc_info);
type_register_static(&aspeed_2700_intc_info);
type_register_static(&aspeed_intcio_info);
type_register_static(&aspeed_2700_intcio_info);
}
static void aspeed_2700_intcio_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
AspeedINTCClass *aic = ASPEED_INTC_CLASS(klass);
dc->desc = "ASPEED 2700 INTC IO Controller";
}
static const TypeInfo aspeed_2700_intcio_info = {
.name = TYPE_ASPEED_2700_INTCIO,
.parent = TYPE_ASPEED_INTCIO,
.class_init = aspeed_2700_intcio_class_init,
};
static void aspeed_intcio_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
dc->desc = "ASPEED INTC IO Controller";
dc->realize = aspeed_intcio_realize;
device_class_set_legacy_reset(dc, aspeed_intcio_reset);
dc->vmsd = NULL;
}
static const TypeInfo aspeed_intcio_info = {
.name = TYPE_ASPEED_INTCIO,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_init = aspeed_intcio_instance_init,
.instance_size = sizeof(AspeedINTCIOState),
.class_init = aspeed_intcio_class_init,
.class_size = sizeof(AspeedINTCIOClass),
.abstract = true,
};
static void aspeed_intcio_realize(DeviceState *dev, Error **errp)
{
memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_intcio_ops, s,
TYPE_ASPEED_INTCIO ".regs", ASPEED_INTC_NR_REGS << 2);
}
static void aspeed_intcio_reset(DeviceState *dev)
{
}
static void aspeed_intcio_instance_init(Object *obj)
{
}
I want to create aspeed_intcio_read and aspeed_intcio_write call back functions.
static const MemoryRegionOps aspeed_intcio_ops = {
.read = aspeed_intcio_read,
.write = aspeed_intcio_write,
.endianness = DEVICE_LITTLE_ENDIAN,
.valid = {
.min_access_size = 4,
.max_access_size = 4,
}
};
Thanks-Jamin
>
> Please send a v2, splitting your series in 3 as requested in the other email.
>
Will resend
> Thanks,
>
> C.
