Re: [PATCH v1 01/18] hw/intc/aspeed: Rename INTC to INTC0

[Cédric Le Goater]

On 2/4/25 07:50, Jamin Lin wrote:
> Hi Cedric, Andrew
>
> > From: Andrew Jeffery <andrew@codeconstruct.com.au>
> > Sent: Thursday, January 30, 2025 11:22 AM
> > To: Cédric Le Goater <clg@kaod.org>; Jamin Lin <jamin_lin@aspeedtech.com>;
> > Peter Maydell <peter.maydell@linaro.org>; Steven Lee
> > <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Joel Stanley
> > <joel@jms.id.au>; open list:ASPEED BMCs <qemu-arm@nongnu.org>; open
> > list:All patches CC here <qemu-devel@nongnu.org>
> > Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang
> > <yunlin.tang@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
> > el@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.

Please send a v2, splitting your series in 3 as requested in
the other email.

Thanks,

C.