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.
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
hw/arm/aspeed_ast27x0.c | 6 +--
hw/intc/aspeed_intc.c | 90 +++++++++++++++++------------------
include/hw/arm/aspeed_soc.h | 2 +-
include/hw/intc/aspeed_intc.h | 2 +-
4 files changed, 50 insertions(+), 50 deletions(-)
diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 4114e15ddd..ba461fcd3c 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -56,7 +56,7 @@ static const hwaddr aspeed_soc_ast2700_memmap[] = {
[ASPEED_DEV_ETH2] = 0x14060000,
[ASPEED_DEV_ETH3] = 0x14070000,
[ASPEED_DEV_EMMC] = 0x12090000,
- [ASPEED_DEV_INTC] = 0x12100000,
+ [ASPEED_DEV_INTC0] = 0x12100000,
[ASPEED_DEV_SLI] = 0x12C17000,
[ASPEED_DEV_SLIIO] = 0x14C1E000,
[ASPEED_GIC_DIST] = 0x12200000,
@@ -372,7 +372,7 @@ static void aspeed_soc_ast2700_init(Object *obj)
object_initialize_child(obj, "sli", &s->sli, TYPE_ASPEED_2700_SLI);
object_initialize_child(obj, "sliio", &s->sliio, TYPE_ASPEED_2700_SLIIO);
- object_initialize_child(obj, "intc", &a->intc, TYPE_ASPEED_2700_INTC);
+ object_initialize_child(obj, "intc", &a->intc, TYPE_ASPEED_2700_INTC0);
snprintf(typename, sizeof(typename), "aspeed.adc-%s", socname);
object_initialize_child(obj, "adc", &s->adc, typename);
@@ -517,7 +517,7 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
}
aspeed_mmio_map(s, SYS_BUS_DEVICE(&a->intc), 0,
- sc->memmap[ASPEED_DEV_INTC]);
+ sc->memmap[ASPEED_DEV_INTC0]);
/* GICINT orgates -> INTC -> GIC */
for (i = 0; i < ic->num_ints; i++) {
diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c
index 126b711b94..df4da759e1 100644
--- a/hw/intc/aspeed_intc.c
+++ b/hw/intc/aspeed_intc.c
@@ -14,27 +14,27 @@
#include "hw/registerfields.h"
#include "qapi/error.h"
-/* INTC Registers */
-REG32(GICINT128_EN, 0x1000)
-REG32(GICINT128_STATUS, 0x1004)
-REG32(GICINT129_EN, 0x1100)
-REG32(GICINT129_STATUS, 0x1104)
-REG32(GICINT130_EN, 0x1200)
-REG32(GICINT130_STATUS, 0x1204)
-REG32(GICINT131_EN, 0x1300)
-REG32(GICINT131_STATUS, 0x1304)
-REG32(GICINT132_EN, 0x1400)
-REG32(GICINT132_STATUS, 0x1404)
-REG32(GICINT133_EN, 0x1500)
-REG32(GICINT133_STATUS, 0x1504)
-REG32(GICINT134_EN, 0x1600)
-REG32(GICINT134_STATUS, 0x1604)
-REG32(GICINT135_EN, 0x1700)
-REG32(GICINT135_STATUS, 0x1704)
-REG32(GICINT136_EN, 0x1800)
-REG32(GICINT136_STATUS, 0x1804)
-
-#define GICINT_STATUS_BASE R_GICINT128_STATUS
+/* AST2700 INTC0 Registers */
+REG32(INTC0_GICINT128_EN, 0x1000)
+REG32(INTC0_GICINT128_STATUS, 0x1004)
+REG32(INTC0_GICINT129_EN, 0x1100)
+REG32(INTC0_GICINT129_STATUS, 0x1104)
+REG32(INTC0_GICINT130_EN, 0x1200)
+REG32(INTC0_GICINT130_STATUS, 0x1204)
+REG32(INTC0_GICINT131_EN, 0x1300)
+REG32(INTC0_GICINT131_STATUS, 0x1304)
+REG32(INTC0_GICINT132_EN, 0x1400)
+REG32(INTC0_GICINT132_STATUS, 0x1404)
+REG32(INTC0_GICINT133_EN, 0x1500)
+REG32(INTC0_GICINT133_STATUS, 0x1504)
+REG32(INTC0_GICINT134_EN, 0x1600)
+REG32(INTC0_GICINT134_STATUS, 0x1604)
+REG32(INTC0_GICINT135_EN, 0x1700)
+REG32(INTC0_GICINT135_STATUS, 0x1704)
+REG32(INTC0_GICINT136_EN, 0x1800)
+REG32(INTC0_GICINT136_STATUS, 0x1804)
+
+#define GICINT_STATUS_BASE R_INTC0_GICINT128_STATUS
static void aspeed_intc_update(AspeedINTCState *s, int irq, int level)
{
@@ -153,15 +153,15 @@ static void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
trace_aspeed_intc_write(offset, size, data);
switch (addr) {
- case R_GICINT128_EN:
- case R_GICINT129_EN:
- case R_GICINT130_EN:
- case R_GICINT131_EN:
- case R_GICINT132_EN:
- case R_GICINT133_EN:
- case R_GICINT134_EN:
- case R_GICINT135_EN:
- case R_GICINT136_EN:
+ case R_INTC0_GICINT128_EN:
+ case R_INTC0_GICINT129_EN:
+ case R_INTC0_GICINT130_EN:
+ case R_INTC0_GICINT131_EN:
+ case R_INTC0_GICINT132_EN:
+ case R_INTC0_GICINT133_EN:
+ case R_INTC0_GICINT134_EN:
+ case R_INTC0_GICINT135_EN:
+ case R_INTC0_GICINT136_EN:
irq = (offset & 0x0f00) >> 8;
if (irq >= aic->num_ints) {
@@ -202,15 +202,15 @@ static void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
}
s->regs[addr] = data;
break;
- case R_GICINT128_STATUS:
- case R_GICINT129_STATUS:
- case R_GICINT130_STATUS:
- case R_GICINT131_STATUS:
- case R_GICINT132_STATUS:
- case R_GICINT133_STATUS:
- case R_GICINT134_STATUS:
- case R_GICINT135_STATUS:
- case R_GICINT136_STATUS:
+ case R_INTC0_GICINT128_STATUS:
+ case R_INTC0_GICINT129_STATUS:
+ case R_INTC0_GICINT130_STATUS:
+ case R_INTC0_GICINT131_STATUS:
+ case R_INTC0_GICINT132_STATUS:
+ case R_INTC0_GICINT133_STATUS:
+ case R_INTC0_GICINT134_STATUS:
+ case R_INTC0_GICINT135_STATUS:
+ case R_INTC0_GICINT136_STATUS:
irq = (offset & 0x0f00) >> 8;
if (irq >= aic->num_ints) {
@@ -336,26 +336,26 @@ static const TypeInfo aspeed_intc_info = {
.abstract = true,
};
-static void aspeed_2700_intc_class_init(ObjectClass *klass, void *data)
+static void aspeed_2700_intc0_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
AspeedINTCClass *aic = ASPEED_INTC_CLASS(klass);
- dc->desc = "ASPEED 2700 INTC Controller";
+ dc->desc = "ASPEED 2700 INTC 0 Controller";
aic->num_lines = 32;
aic->num_ints = 9;
}
-static const TypeInfo aspeed_2700_intc_info = {
- .name = TYPE_ASPEED_2700_INTC,
+static const TypeInfo aspeed_2700_intc0_info = {
+ .name = TYPE_ASPEED_2700_INTC0,
.parent = TYPE_ASPEED_INTC,
- .class_init = aspeed_2700_intc_class_init,
+ .class_init = aspeed_2700_intc0_class_init,
};
static void aspeed_intc_register_types(void)
{
type_register_static(&aspeed_intc_info);
- type_register_static(&aspeed_2700_intc_info);
+ type_register_static(&aspeed_2700_intc0_info);
}
type_init(aspeed_intc_register_types);
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 689f52dae8..51e585e3e4 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -194,7 +194,7 @@ enum {
ASPEED_DEV_EHCI1,
ASPEED_DEV_EHCI2,
ASPEED_DEV_VIC,
- ASPEED_DEV_INTC,
+ ASPEED_DEV_INTC0,
ASPEED_DEV_SDMC,
ASPEED_DEV_SCU,
ASPEED_DEV_ADC,
diff --git a/include/hw/intc/aspeed_intc.h b/include/hw/intc/aspeed_intc.h
index 18cb43476c..10718ed4a1 100644
--- a/include/hw/intc/aspeed_intc.h
+++ b/include/hw/intc/aspeed_intc.h
@@ -13,7 +13,7 @@
#include "hw/or-irq.h"
#define TYPE_ASPEED_INTC "aspeed.intc"
-#define TYPE_ASPEED_2700_INTC TYPE_ASPEED_INTC "-ast2700"
+#define TYPE_ASPEED_2700_INTC0 TYPE_ASPEED_INTC "0" "-ast2700"
OBJECT_DECLARE_TYPE(AspeedINTCState, AspeedINTCClass, ASPEED_INTC)
#define ASPEED_INTC_NR_REGS (0x2000 >> 2)
--
2.34.1On Tue, 2025-01-21 at 15:04 +0800, 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.
>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
> hw/arm/aspeed_ast27x0.c | 6 +--
> hw/intc/aspeed_intc.c | 90 +++++++++++++++++----------------
> --
> include/hw/arm/aspeed_soc.h | 2 +-
> include/hw/intc/aspeed_intc.h | 2 +-
> 4 files changed, 50 insertions(+), 50 deletions(-)
>
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> index 4114e15ddd..ba461fcd3c 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -56,7 +56,7 @@ static const hwaddr aspeed_soc_ast2700_memmap[] = {
> [ASPEED_DEV_ETH2] = 0x14060000,
> [ASPEED_DEV_ETH3] = 0x14070000,
> [ASPEED_DEV_EMMC] = 0x12090000,
> - [ASPEED_DEV_INTC] = 0x12100000,
> + [ASPEED_DEV_INTC0] = 0x12100000,
> [ASPEED_DEV_SLI] = 0x12C17000,
> [ASPEED_DEV_SLIIO] = 0x14C1E000,
> [ASPEED_GIC_DIST] = 0x12200000,
> @@ -372,7 +372,7 @@ static void aspeed_soc_ast2700_init(Object *obj)
>
> object_initialize_child(obj, "sli", &s->sli,
> TYPE_ASPEED_2700_SLI);
> object_initialize_child(obj, "sliio", &s->sliio,
> TYPE_ASPEED_2700_SLIIO);
> - object_initialize_child(obj, "intc", &a->intc,
> TYPE_ASPEED_2700_INTC);
> + object_initialize_child(obj, "intc", &a->intc,
> TYPE_ASPEED_2700_INTC0);
Shouldn't we also change the propname to "intc0" (... if we're to
continue with that style of naming)?
Andrew
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 ?
Thanks,
C.
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
> hw/arm/aspeed_ast27x0.c | 6 +--
> hw/intc/aspeed_intc.c | 90 +++++++++++++++++------------------
> include/hw/arm/aspeed_soc.h | 2 +-
> include/hw/intc/aspeed_intc.h | 2 +-
> 4 files changed, 50 insertions(+), 50 deletions(-)
>
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> index 4114e15ddd..ba461fcd3c 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -56,7 +56,7 @@ static const hwaddr aspeed_soc_ast2700_memmap[] = {
> [ASPEED_DEV_ETH2] = 0x14060000,
> [ASPEED_DEV_ETH3] = 0x14070000,
> [ASPEED_DEV_EMMC] = 0x12090000,
> - [ASPEED_DEV_INTC] = 0x12100000,
> + [ASPEED_DEV_INTC0] = 0x12100000,
> [ASPEED_DEV_SLI] = 0x12C17000,
> [ASPEED_DEV_SLIIO] = 0x14C1E000,
> [ASPEED_GIC_DIST] = 0x12200000,
> @@ -372,7 +372,7 @@ static void aspeed_soc_ast2700_init(Object *obj)
>
> object_initialize_child(obj, "sli", &s->sli, TYPE_ASPEED_2700_SLI);
> object_initialize_child(obj, "sliio", &s->sliio, TYPE_ASPEED_2700_SLIIO);
> - object_initialize_child(obj, "intc", &a->intc, TYPE_ASPEED_2700_INTC);
> + object_initialize_child(obj, "intc", &a->intc, TYPE_ASPEED_2700_INTC0);
>
> snprintf(typename, sizeof(typename), "aspeed.adc-%s", socname);
> object_initialize_child(obj, "adc", &s->adc, typename);
> @@ -517,7 +517,7 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
> }
>
> aspeed_mmio_map(s, SYS_BUS_DEVICE(&a->intc), 0,
> - sc->memmap[ASPEED_DEV_INTC]);
> + sc->memmap[ASPEED_DEV_INTC0]);
>
> /* GICINT orgates -> INTC -> GIC */
> for (i = 0; i < ic->num_ints; i++) {
> diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c
> index 126b711b94..df4da759e1 100644
> --- a/hw/intc/aspeed_intc.c
> +++ b/hw/intc/aspeed_intc.c
> @@ -14,27 +14,27 @@
> #include "hw/registerfields.h"
> #include "qapi/error.h"
>
> -/* INTC Registers */
> -REG32(GICINT128_EN, 0x1000)
> -REG32(GICINT128_STATUS, 0x1004)
> -REG32(GICINT129_EN, 0x1100)
> -REG32(GICINT129_STATUS, 0x1104)
> -REG32(GICINT130_EN, 0x1200)
> -REG32(GICINT130_STATUS, 0x1204)
> -REG32(GICINT131_EN, 0x1300)
> -REG32(GICINT131_STATUS, 0x1304)
> -REG32(GICINT132_EN, 0x1400)
> -REG32(GICINT132_STATUS, 0x1404)
> -REG32(GICINT133_EN, 0x1500)
> -REG32(GICINT133_STATUS, 0x1504)
> -REG32(GICINT134_EN, 0x1600)
> -REG32(GICINT134_STATUS, 0x1604)
> -REG32(GICINT135_EN, 0x1700)
> -REG32(GICINT135_STATUS, 0x1704)
> -REG32(GICINT136_EN, 0x1800)
> -REG32(GICINT136_STATUS, 0x1804)
> -
> -#define GICINT_STATUS_BASE R_GICINT128_STATUS
> +/* AST2700 INTC0 Registers */
> +REG32(INTC0_GICINT128_EN, 0x1000)
> +REG32(INTC0_GICINT128_STATUS, 0x1004)
> +REG32(INTC0_GICINT129_EN, 0x1100)
> +REG32(INTC0_GICINT129_STATUS, 0x1104)
> +REG32(INTC0_GICINT130_EN, 0x1200)
> +REG32(INTC0_GICINT130_STATUS, 0x1204)
> +REG32(INTC0_GICINT131_EN, 0x1300)
> +REG32(INTC0_GICINT131_STATUS, 0x1304)
> +REG32(INTC0_GICINT132_EN, 0x1400)
> +REG32(INTC0_GICINT132_STATUS, 0x1404)
> +REG32(INTC0_GICINT133_EN, 0x1500)
> +REG32(INTC0_GICINT133_STATUS, 0x1504)
> +REG32(INTC0_GICINT134_EN, 0x1600)
> +REG32(INTC0_GICINT134_STATUS, 0x1604)
> +REG32(INTC0_GICINT135_EN, 0x1700)
> +REG32(INTC0_GICINT135_STATUS, 0x1704)
> +REG32(INTC0_GICINT136_EN, 0x1800)
> +REG32(INTC0_GICINT136_STATUS, 0x1804)
> +
> +#define GICINT_STATUS_BASE R_INTC0_GICINT128_STATUS
>
> static void aspeed_intc_update(AspeedINTCState *s, int irq, int level)
> {
> @@ -153,15 +153,15 @@ static void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
> trace_aspeed_intc_write(offset, size, data);
>
> switch (addr) {
> - case R_GICINT128_EN:
> - case R_GICINT129_EN:
> - case R_GICINT130_EN:
> - case R_GICINT131_EN:
> - case R_GICINT132_EN:
> - case R_GICINT133_EN:
> - case R_GICINT134_EN:
> - case R_GICINT135_EN:
> - case R_GICINT136_EN:
> + case R_INTC0_GICINT128_EN:
> + case R_INTC0_GICINT129_EN:
> + case R_INTC0_GICINT130_EN:
> + case R_INTC0_GICINT131_EN:
> + case R_INTC0_GICINT132_EN:
> + case R_INTC0_GICINT133_EN:
> + case R_INTC0_GICINT134_EN:
> + case R_INTC0_GICINT135_EN:
> + case R_INTC0_GICINT136_EN:
> irq = (offset & 0x0f00) >> 8;
>
> if (irq >= aic->num_ints) {
> @@ -202,15 +202,15 @@ static void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
> }
> s->regs[addr] = data;
> break;
> - case R_GICINT128_STATUS:
> - case R_GICINT129_STATUS:
> - case R_GICINT130_STATUS:
> - case R_GICINT131_STATUS:
> - case R_GICINT132_STATUS:
> - case R_GICINT133_STATUS:
> - case R_GICINT134_STATUS:
> - case R_GICINT135_STATUS:
> - case R_GICINT136_STATUS:
> + case R_INTC0_GICINT128_STATUS:
> + case R_INTC0_GICINT129_STATUS:
> + case R_INTC0_GICINT130_STATUS:
> + case R_INTC0_GICINT131_STATUS:
> + case R_INTC0_GICINT132_STATUS:
> + case R_INTC0_GICINT133_STATUS:
> + case R_INTC0_GICINT134_STATUS:
> + case R_INTC0_GICINT135_STATUS:
> + case R_INTC0_GICINT136_STATUS:
> irq = (offset & 0x0f00) >> 8;
>
> if (irq >= aic->num_ints) {
> @@ -336,26 +336,26 @@ static const TypeInfo aspeed_intc_info = {
> .abstract = true,
> };
>
> -static void aspeed_2700_intc_class_init(ObjectClass *klass, void *data)
> +static void aspeed_2700_intc0_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> AspeedINTCClass *aic = ASPEED_INTC_CLASS(klass);
>
> - dc->desc = "ASPEED 2700 INTC Controller";
> + dc->desc = "ASPEED 2700 INTC 0 Controller";
> aic->num_lines = 32;
> aic->num_ints = 9;
> }
>
> -static const TypeInfo aspeed_2700_intc_info = {
> - .name = TYPE_ASPEED_2700_INTC,
> +static const TypeInfo aspeed_2700_intc0_info = {
> + .name = TYPE_ASPEED_2700_INTC0,
> .parent = TYPE_ASPEED_INTC,
> - .class_init = aspeed_2700_intc_class_init,
> + .class_init = aspeed_2700_intc0_class_init,
> };
>
> static void aspeed_intc_register_types(void)
> {
> type_register_static(&aspeed_intc_info);
> - type_register_static(&aspeed_2700_intc_info);
> + type_register_static(&aspeed_2700_intc0_info);
> }
>
> type_init(aspeed_intc_register_types);
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index 689f52dae8..51e585e3e4 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -194,7 +194,7 @@ enum {
> ASPEED_DEV_EHCI1,
> ASPEED_DEV_EHCI2,
> ASPEED_DEV_VIC,
> - ASPEED_DEV_INTC,
> + ASPEED_DEV_INTC0,
> ASPEED_DEV_SDMC,
> ASPEED_DEV_SCU,
> ASPEED_DEV_ADC,
> diff --git a/include/hw/intc/aspeed_intc.h b/include/hw/intc/aspeed_intc.h
> index 18cb43476c..10718ed4a1 100644
> --- a/include/hw/intc/aspeed_intc.h
> +++ b/include/hw/intc/aspeed_intc.h
> @@ -13,7 +13,7 @@
> #include "hw/or-irq.h"
>
> #define TYPE_ASPEED_INTC "aspeed.intc"
> -#define TYPE_ASPEED_2700_INTC TYPE_ASPEED_INTC "-ast2700"
> +#define TYPE_ASPEED_2700_INTC0 TYPE_ASPEED_INTC "0" "-ast2700"
> OBJECT_DECLARE_TYPE(AspeedINTCState, AspeedINTCClass, ASPEED_INTC)
>
> #define ASPEED_INTC_NR_REGS (0x2000 >> 2)
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.camel@codeconstruct.com.au/ Ryan didn't like deviating from their internal documentation :( Andrew
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
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.
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
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.
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.
>
© 2016 - 2025 Red Hat, Inc.