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

[Jamin Lin]

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.

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?
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.

Thanks-Jamin