qemu-arm
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Cédric Le Goater
Subject: Re: [PATCH v1 01/18] hw/intc/aspeed: Rename INTC to INTC0
Date: Tue, 4 Feb 2025 11:26:27 +0100
User-agent: Mozilla Thunderbird 
On 2/4/25 09:22, Jamin Lin wrote:

Hi Cedric,


From: Cédric Le Goater <clg@kaod.org>
Sent: Tuesday, February 4, 2025 3:35 PM
To: Jamin Lin <jamin_lin@aspeedtech.com>; Andrew Jeffery
<andrew@codeconstruct.com.au>; 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 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.



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


can you define them as

INTC_xxx

to avoid all the duplication below ?


Thanks,

C.




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.
reply via email to
[Prev in Thread] Current Thread [Next in Thread]