According to the AST2700 datasheet, the INTC(CPU DIE) controller has 16KB
(0x4000) of register space, and the INTCIO (I/O DIE) controller has 1KB (0x400)
of register space.
Introduced a new class attribute "mem_size" to set different memory sizes for
the INTC models in AST2700.
Introduced a new class attribute "reg_size" to set different register sizes for
the INTC models in AST2700.
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
hw/intc/aspeed_intc.c | 17 +++++++++++++----
include/hw/intc/aspeed_intc.h | 4 ++++
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c
index 126b711b94..316885a27a 100644
--- a/hw/intc/aspeed_intc.c
+++ b/hw/intc/aspeed_intc.c
@@ -117,10 +117,11 @@ static void aspeed_intc_set_irq(void *opaque, int irq, int level)
static uint64_t aspeed_intc_read(void *opaque, hwaddr offset, unsigned int size)
{
AspeedINTCState *s = ASPEED_INTC(opaque);
+ AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
uint32_t addr = offset >> 2;
uint32_t value = 0;
- if (addr >= ASPEED_INTC_NR_REGS) {
+ if (offset >= aic->reg_size) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
__func__, offset);
@@ -143,7 +144,7 @@ static void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
uint32_t change;
uint32_t irq;
- if (addr >= ASPEED_INTC_NR_REGS) {
+ if (offset >= aic->reg_size) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
__func__, offset);
@@ -302,10 +303,16 @@ static void aspeed_intc_realize(DeviceState *dev, Error **errp)
AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
int i;
+ memory_region_init(&s->iomem_container, OBJECT(s),
+ TYPE_ASPEED_INTC ".container", aic->mem_size);
+
+ sysbus_init_mmio(sbd, &s->iomem_container);
+
memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_intc_ops, s,
- TYPE_ASPEED_INTC ".regs", ASPEED_INTC_NR_REGS << 2);
+ TYPE_ASPEED_INTC ".regs", aic->reg_size);
+
+ memory_region_add_subregion(&s->iomem_container, 0x0, &s->iomem);
- sysbus_init_mmio(sbd, &s->iomem);
qdev_init_gpio_in(dev, aspeed_intc_set_irq, aic->num_ints);
for (i = 0; i < aic->num_ints; i++) {
@@ -344,6 +351,8 @@ static void aspeed_2700_intc_class_init(ObjectClass *klass, void *data)
dc->desc = "ASPEED 2700 INTC Controller";
aic->num_lines = 32;
aic->num_ints = 9;
+ aic->mem_size = 0x4000;
+ aic->reg_size = 0x2000;
}
static const TypeInfo aspeed_2700_intc_info = {
diff --git a/include/hw/intc/aspeed_intc.h b/include/hw/intc/aspeed_intc.h
index 18cb43476c..ecaeb15aea 100644
--- a/include/hw/intc/aspeed_intc.h
+++ b/include/hw/intc/aspeed_intc.h
@@ -25,6 +25,8 @@ struct AspeedINTCState {
/*< public >*/
MemoryRegion iomem;
+ MemoryRegion iomem_container;
+
uint32_t regs[ASPEED_INTC_NR_REGS];
OrIRQState orgates[ASPEED_INTC_NR_INTS];
qemu_irq output_pins[ASPEED_INTC_NR_INTS];
@@ -39,6 +41,8 @@ struct AspeedINTCClass {
uint32_t num_lines;
uint32_t num_ints;
+ uint64_t mem_size;
+ uint64_t reg_size;
};
#endif /* ASPEED_INTC_H */
--
2.34.1Hello Jamin,
On 2/13/25 04:35, Jamin Lin wrote:
> According to the AST2700 datasheet, the INTC(CPU DIE) controller has 16KB
> (0x4000) of register space, and the INTCIO (I/O DIE) controller has 1KB (0x400)
> of register space.
>
> Introduced a new class attribute "mem_size" to set different memory sizes for
> the INTC models in AST2700.
>
> Introduced a new class attribute "reg_size" to set different register sizes for
> the INTC models in AST2700.
Shouldn't that be multiple patches ?
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
> hw/intc/aspeed_intc.c | 17 +++++++++++++----
> include/hw/intc/aspeed_intc.h | 4 ++++
> 2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c
> index 126b711b94..316885a27a 100644
> --- a/hw/intc/aspeed_intc.c
> +++ b/hw/intc/aspeed_intc.c
> @@ -117,10 +117,11 @@ static void aspeed_intc_set_irq(void *opaque, int irq, int level)
> static uint64_t aspeed_intc_read(void *opaque, hwaddr offset, unsigned int size)
> {
> AspeedINTCState *s = ASPEED_INTC(opaque);
> + AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> uint32_t addr = offset >> 2;
> uint32_t value = 0;
>
> - if (addr >= ASPEED_INTC_NR_REGS) {
Side note, ASPEED_INTC_NR_REGS is defined as
#define ASPEED_INTC_NR_REGS (0x2000 >> 2)
and the register array as:
uint32_t regs[ASPEED_INTC_NR_REGS];
The number of regs looks pretty big for me. Are the registers covering
the whole MMIO aperture ?
> + if (offset >= aic->reg_size) {
This is dead code since the MMIO aperture has the same size. You could
remove the check.
> qemu_log_mask(LOG_GUEST_ERROR,
> "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
> __func__, offset);
> @@ -143,7 +144,7 @@ static void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
> uint32_t change;
> uint32_t irq;
>
> - if (addr >= ASPEED_INTC_NR_REGS) {
> + if (offset >= aic->reg_size) {
> qemu_log_mask(LOG_GUEST_ERROR,
> "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
> __func__, offset);
> @@ -302,10 +303,16 @@ static void aspeed_intc_realize(DeviceState *dev, Error **errp)
> AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> int i;
>
> + memory_region_init(&s->iomem_container, OBJECT(s),
> + TYPE_ASPEED_INTC ".container", aic->mem_size);
> +
> + sysbus_init_mmio(sbd, &s->iomem_container);
Why introduce a container ? Do you plan to have multiple sub-regions ?
Thanks,
C.
> +
> memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_intc_ops, s,
> - TYPE_ASPEED_INTC ".regs", ASPEED_INTC_NR_REGS << 2);
> + TYPE_ASPEED_INTC ".regs", aic->reg_size);
> +
> + memory_region_add_subregion(&s->iomem_container, 0x0, &s->iomem);
>
> - sysbus_init_mmio(sbd, &s->iomem);
> qdev_init_gpio_in(dev, aspeed_intc_set_irq, aic->num_ints);
>
> for (i = 0; i < aic->num_ints; i++) {
> @@ -344,6 +351,8 @@ static void aspeed_2700_intc_class_init(ObjectClass *klass, void *data)
> dc->desc = "ASPEED 2700 INTC Controller";
> aic->num_lines = 32;
> aic->num_ints = 9;
> + aic->mem_size = 0x4000;
> + aic->reg_size = 0x2000;
> }
>
> static const TypeInfo aspeed_2700_intc_info = {
> diff --git a/include/hw/intc/aspeed_intc.h b/include/hw/intc/aspeed_intc.h
> index 18cb43476c..ecaeb15aea 100644
> --- a/include/hw/intc/aspeed_intc.h
> +++ b/include/hw/intc/aspeed_intc.h
> @@ -25,6 +25,8 @@ struct AspeedINTCState {
>
> /*< public >*/
> MemoryRegion iomem;
> + MemoryRegion iomem_container;
> +
> uint32_t regs[ASPEED_INTC_NR_REGS];
> OrIRQState orgates[ASPEED_INTC_NR_INTS];
> qemu_irq output_pins[ASPEED_INTC_NR_INTS];
> @@ -39,6 +41,8 @@ struct AspeedINTCClass {
>
> uint32_t num_lines;
> uint32_t num_ints;
> + uint64_t mem_size;
> + uint64_t reg_size;
> };
>
> #endif /* ASPEED_INTC_H */
Hi Cedric,
> Subject: Re: [PATCH v3 01/28] hw/intc/aspeed: Support setting different
> memory and register size
>
> Hello Jamin,
>
> On 2/13/25 04:35, Jamin Lin wrote:
> > According to the AST2700 datasheet, the INTC(CPU DIE) controller has
> > 16KB
> > (0x4000) of register space, and the INTCIO (I/O DIE) controller has
> > 1KB (0x400) of register space.
> >
> > Introduced a new class attribute "mem_size" to set different memory
> > sizes for the INTC models in AST2700.
> >
> > Introduced a new class attribute "reg_size" to set different register
> > sizes for the INTC models in AST2700.
>
> Shouldn't that be multiple patches ?
>
I will add one patch for reg_size and another for mem_size.
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> > hw/intc/aspeed_intc.c | 17 +++++++++++++----
> > include/hw/intc/aspeed_intc.h | 4 ++++
> > 2 files changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c index
> > 126b711b94..316885a27a 100644
> > --- a/hw/intc/aspeed_intc.c
> > +++ b/hw/intc/aspeed_intc.c
> > @@ -117,10 +117,11 @@ static void aspeed_intc_set_irq(void *opaque, int
> irq, int level)
> > static uint64_t aspeed_intc_read(void *opaque, hwaddr offset, unsigned
> int size)
> > {
> > AspeedINTCState *s = ASPEED_INTC(opaque);
> > + AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> > uint32_t addr = offset >> 2;
> > uint32_t value = 0;
> >
> > - if (addr >= ASPEED_INTC_NR_REGS) {
>
> Side note, ASPEED_INTC_NR_REGS is defined as
>
> #define ASPEED_INTC_NR_REGS (0x2000 >> 2)
>
> and the register array as:
>
> uint32_t regs[ASPEED_INTC_NR_REGS];
>
> The number of regs looks pretty big for me. Are the registers covering the
> whole MMIO aperture ?
>
According to the datasheet, the entire register address space size of INTC (CPU DIE) is 16KB
(0x12100000-0x12103FFF). Therefore, I set the memory size to 0x4000.
Currently, we need to use the "GICINT192-201 raw status and clear" register INTC1B04.
Thus, an array size of 0x2000 is sufficient.
However, we are going to increase the size to 0x3000 to support the co-processors SSP and TSP
In the another patch series.
We also need to include the following register definitions:
/* SSP INTC Registers */
REG32(SSPINT128_EN, 0x2000)
REG32(SSPINT128_STATUS, 0x2004)
REG32(SSPINT129_EN, 0x2100)
REG32(SSPINT129_STATUS, 0x2104)
REG32(SSPINT130_EN, 0x2200)
REG32(SSPINT130_STATUS, 0x2204)
REG32(SSPINT131_EN, 0x2300)
REG32(SSPINT131_STATUS, 0x2304)
REG32(SSPINT132_EN, 0x2400)
REG32(SSPINT132_STATUS, 0x2404)
REG32(SSPINT133_EN, 0x2500)
REG32(SSPINT133_STATUS, 0x2504)
REG32(SSPINT134_EN, 0x2600)
REG32(SSPINT134_STATUS, 0x2604)
REG32(SSPINT135_EN, 0x2700)
REG32(SSPINT135_STATUS, 0x2704)
REG32(SSPINT136_EN, 0x2800)
REG32(SSPINT136_STATUS, 0x2804)
REG32(SSPINT137_EN, 0x2900)
REG32(SSPINT137_STATUS, 0x2904)
REG32(SSPINT138_EN, 0x2A00)
REG32(SSPINT138_STATUS, 0x2A04)
REG32(SSPINT160_169_EN, 0x2B00)
REG32(SSPINT160_169_STATUS, 0x2B04)
>
> > + if (offset >= aic->reg_size) {
>
> This is dead code since the MMIO aperture has the same size. You could
> remove the check.
Will remove.
>
> > qemu_log_mask(LOG_GUEST_ERROR,
> > "%s: Out-of-bounds read at offset 0x%"
> HWADDR_PRIx "\n",
> > __func__, offset); @@ -143,7 +144,7 @@
> static
> > void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
> > uint32_t change;
> > uint32_t irq;
> >
> > - if (addr >= ASPEED_INTC_NR_REGS) {
> > + if (offset >= aic->reg_size) {
> > qemu_log_mask(LOG_GUEST_ERROR,
> > "%s: Out-of-bounds write at offset 0x%"
> HWADDR_PRIx "\n",
> > __func__, offset); @@ -302,10 +303,16 @@
> > static void aspeed_intc_realize(DeviceState *dev, Error **errp)
> > AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> > int i;
> >
> > + memory_region_init(&s->iomem_container, OBJECT(s),
> > + TYPE_ASPEED_INTC ".container", aic->mem_size);
> > +
> > + sysbus_init_mmio(sbd, &s->iomem_container);
>
> Why introduce a container ? Do you plan to have multiple sub-regions ?
>
I created a container to save the entire register address space of INTC and its sub-region to place the
actual used register address space.
0000000012100000-0000000012103fff (prio 0, i/o): aspeed.intc.container
0000000012100000-0000000012101fff (prio 0, i/o): aspeed.intc.regs
0000000014c18000-0000000014c183ff (prio 0, i/o): aspeed.intc.container
0000000014c18000-0000000014c183d7 (prio 0, i/o): aspeed.intc.regs
If I misunderstood this design, I will change it to have two memory regions for INTC and INTCIO, respectively.
If need, I will change to the following memory regions. --> it removes containers.
0000000012100000-0000000012101fff (prio 0, i/o): aspeed.intc.regs
0000000014c18000-0000000014c183d7 (prio 0, i/o): aspeed.intc.regs
Do you expect the above memory regions for INTC?
Thanks for your review and suggestion.
Jamin
>
> Thanks,
>
> C.
>
>
>
> > +
> > memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_intc_ops,
> s,
> > - TYPE_ASPEED_INTC ".regs",
> ASPEED_INTC_NR_REGS << 2);
> > + TYPE_ASPEED_INTC ".regs", aic->reg_size);
> > +
> > + memory_region_add_subregion(&s->iomem_container, 0x0,
> &s->iomem);
> >
> > - sysbus_init_mmio(sbd, &s->iomem);
> > qdev_init_gpio_in(dev, aspeed_intc_set_irq, aic->num_ints);
> >
> > for (i = 0; i < aic->num_ints; i++) { @@ -344,6 +351,8 @@ static
> > void aspeed_2700_intc_class_init(ObjectClass *klass, void *data)
> > dc->desc = "ASPEED 2700 INTC Controller";
> > aic->num_lines = 32;
> > aic->num_ints = 9;
> > + aic->mem_size = 0x4000;
> > + aic->reg_size = 0x2000;
> > }
> >
> > static const TypeInfo aspeed_2700_intc_info = { diff --git
> > a/include/hw/intc/aspeed_intc.h b/include/hw/intc/aspeed_intc.h index
> > 18cb43476c..ecaeb15aea 100644
> > --- a/include/hw/intc/aspeed_intc.h
> > +++ b/include/hw/intc/aspeed_intc.h
> > @@ -25,6 +25,8 @@ struct AspeedINTCState {
> >
> > /*< public >*/
> > MemoryRegion iomem;
> > + MemoryRegion iomem_container;
> > +
> > uint32_t regs[ASPEED_INTC_NR_REGS];
> > OrIRQState orgates[ASPEED_INTC_NR_INTS];
> > qemu_irq output_pins[ASPEED_INTC_NR_INTS]; @@ -39,6 +41,8 @@
> > struct AspeedINTCClass {
> >
> > uint32_t num_lines;
> > uint32_t num_ints;
> > + uint64_t mem_size;
> > + uint64_t reg_size;
> > };
> >
> > #endif /* ASPEED_INTC_H */
On 2/20/25 06:45, Jamin Lin wrote:
> Hi Cedric,
>
>> Subject: Re: [PATCH v3 01/28] hw/intc/aspeed: Support setting different
>> memory and register size
>>
>> Hello Jamin,
>>
>> On 2/13/25 04:35, Jamin Lin wrote:
>>> According to the AST2700 datasheet, the INTC(CPU DIE) controller has
>>> 16KB
>>> (0x4000) of register space, and the INTCIO (I/O DIE) controller has
>>> 1KB (0x400) of register space.
>>>
>>> Introduced a new class attribute "mem_size" to set different memory
>>> sizes for the INTC models in AST2700.
>>>
>>> Introduced a new class attribute "reg_size" to set different register
>>> sizes for the INTC models in AST2700.
>>
>> Shouldn't that be multiple patches ?
>>
>
> I will add one patch for reg_size and another for mem_size.
>
>>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>>> ---
>>> hw/intc/aspeed_intc.c | 17 +++++++++++++----
>>> include/hw/intc/aspeed_intc.h | 4 ++++
>>> 2 files changed, 17 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c index
>>> 126b711b94..316885a27a 100644
>>> --- a/hw/intc/aspeed_intc.c
>>> +++ b/hw/intc/aspeed_intc.c
>>> @@ -117,10 +117,11 @@ static void aspeed_intc_set_irq(void *opaque, int
>> irq, int level)
>>> static uint64_t aspeed_intc_read(void *opaque, hwaddr offset, unsigned
>> int size)
>>> {
>>> AspeedINTCState *s = ASPEED_INTC(opaque);
>>> + AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
>>> uint32_t addr = offset >> 2;
>>> uint32_t value = 0;
>>>
>>> - if (addr >= ASPEED_INTC_NR_REGS) {
>>
>> Side note, ASPEED_INTC_NR_REGS is defined as
>>
>> #define ASPEED_INTC_NR_REGS (0x2000 >> 2)
>>
>> and the register array as:
>>
>> uint32_t regs[ASPEED_INTC_NR_REGS];
>>
>> The number of regs looks pretty big for me. Are the registers covering the
>> whole MMIO aperture ?
>>
> According to the datasheet, the entire register address space size of INTC (CPU DIE) is 16KB
> (0x12100000-0x12103FFF). Therefore, I set the memory size to 0x4000.
> Currently, we need to use the "GICINT192-201 raw status and clear" register INTC1B04.
> Thus, an array size of 0x2000 is sufficient.
yes but we are only using these regs :
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)
REG32(GICINT192_201_EN, 0x1B00)
REG32(GICINT192_201_STATUS, 0x1B04)
so the first 0x1000 are unused.
>
> However, we are going to increase the size to 0x3000 to support the co-processors SSP and TSP
> In the another patch series.
> We also need to include the following register definitions:
>
> /* SSP INTC Registers */
> REG32(SSPINT128_EN, 0x2000)
> REG32(SSPINT128_STATUS, 0x2004)
> REG32(SSPINT129_EN, 0x2100)
> REG32(SSPINT129_STATUS, 0x2104)
> REG32(SSPINT130_EN, 0x2200)
> REG32(SSPINT130_STATUS, 0x2204)
> REG32(SSPINT131_EN, 0x2300)
> REG32(SSPINT131_STATUS, 0x2304)
> REG32(SSPINT132_EN, 0x2400)
> REG32(SSPINT132_STATUS, 0x2404)
> REG32(SSPINT133_EN, 0x2500)
> REG32(SSPINT133_STATUS, 0x2504)
> REG32(SSPINT134_EN, 0x2600)
> REG32(SSPINT134_STATUS, 0x2604)
> REG32(SSPINT135_EN, 0x2700)
> REG32(SSPINT135_STATUS, 0x2704)
> REG32(SSPINT136_EN, 0x2800)
> REG32(SSPINT136_STATUS, 0x2804)
> REG32(SSPINT137_EN, 0x2900)
> REG32(SSPINT137_STATUS, 0x2904)
> REG32(SSPINT138_EN, 0x2A00)
> REG32(SSPINT138_STATUS, 0x2A04)
> REG32(SSPINT160_169_EN, 0x2B00)
> REG32(SSPINT160_169_STATUS, 0x2B04)
>
>>
>>> + if (offset >= aic->reg_size) {
>>
>> This is dead code since the MMIO aperture has the same size. You could
>> remove the check.
>
> Will remove.
>>
>>> qemu_log_mask(LOG_GUEST_ERROR,
>>> "%s: Out-of-bounds read at offset 0x%"
>> HWADDR_PRIx "\n",
>>> __func__, offset); @@ -143,7 +144,7 @@
>> static
>>> void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
>>> uint32_t change;
>>> uint32_t irq;
>>>
>>> - if (addr >= ASPEED_INTC_NR_REGS) {
>>> + if (offset >= aic->reg_size) {
>>> qemu_log_mask(LOG_GUEST_ERROR,
>>> "%s: Out-of-bounds write at offset 0x%"
>> HWADDR_PRIx "\n",
>>> __func__, offset); @@ -302,10 +303,16 @@
>>> static void aspeed_intc_realize(DeviceState *dev, Error **errp)
>>> AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
>>> int i;
>>>
>>> + memory_region_init(&s->iomem_container, OBJECT(s),
>>> + TYPE_ASPEED_INTC ".container", aic->mem_size);
>>> +
>>> + sysbus_init_mmio(sbd, &s->iomem_container);
>>
>> Why introduce a container ? Do you plan to have multiple sub-regions ?
>>
> I created a container to save the entire register address space of INTC and its sub-region to place the
> actual used register address space.
> 0000000012100000-0000000012103fff (prio 0, i/o): aspeed.intc.container
> 0000000012100000-0000000012101fff (prio 0, i/o): aspeed.intc.regs
> 0000000014c18000-0000000014c183ff (prio 0, i/o): aspeed.intc.container
> 0000000014c18000-0000000014c183d7 (prio 0, i/o): aspeed.intc.regs
>
> If I misunderstood this design, I will change it to have two memory regions for INTC and INTCIO, respectively.
> If need, I will change to the following memory regions. --> it removes containers.
> 0000000012100000-0000000012101fff (prio 0, i/o): aspeed.intc.regs
> 0000000014c18000-0000000014c183d7 (prio 0, i/o): aspeed.intc.regs
I don't think the region container is useful in that case since there is
only a single region per model.
However, we could "optimize" a bit the MMIO apertures to avoid mapping
huge unused gaps and only map the useful set of registers :
INTC Registers [ 0x1000 - 0x1B04 ]
SSP INTC Registers [ 0x2000 - 0x2B04 ]
INTCIO Registers [ 0x0100 - 0x0154 ]
Each set would be associated with a subregion which would be mapped at
the right offset in the region container.
This is just a suggestion.
Thanks,
C.
> Do you expect the above memory regions for INTC?
>
> Thanks for your review and suggestion.
> Jamin
>>
>> Thanks,
>>
>> C.
>>
>>
>>
>>> +
>>> memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_intc_ops,
>> s,
>>> - TYPE_ASPEED_INTC ".regs",
>> ASPEED_INTC_NR_REGS << 2);
>>> + TYPE_ASPEED_INTC ".regs", aic->reg_size);
>>> +
>>> + memory_region_add_subregion(&s->iomem_container, 0x0,
>> &s->iomem);
>>>
>>> - sysbus_init_mmio(sbd, &s->iomem);
>>> qdev_init_gpio_in(dev, aspeed_intc_set_irq, aic->num_ints);
>>>
>>> for (i = 0; i < aic->num_ints; i++) { @@ -344,6 +351,8 @@ static
>>> void aspeed_2700_intc_class_init(ObjectClass *klass, void *data)
>>> dc->desc = "ASPEED 2700 INTC Controller";
>>> aic->num_lines = 32;
>>> aic->num_ints = 9;
>>> + aic->mem_size = 0x4000;
>>> + aic->reg_size = 0x2000;
>>> }
>>>
>>> static const TypeInfo aspeed_2700_intc_info = { diff --git
>>> a/include/hw/intc/aspeed_intc.h b/include/hw/intc/aspeed_intc.h index
>>> 18cb43476c..ecaeb15aea 100644
>>> --- a/include/hw/intc/aspeed_intc.h
>>> +++ b/include/hw/intc/aspeed_intc.h
>>> @@ -25,6 +25,8 @@ struct AspeedINTCState {
>>>
>>> /*< public >*/
>>> MemoryRegion iomem;
>>> + MemoryRegion iomem_container;
>>> +
>>> uint32_t regs[ASPEED_INTC_NR_REGS];
>>> OrIRQState orgates[ASPEED_INTC_NR_INTS];
>>> qemu_irq output_pins[ASPEED_INTC_NR_INTS]; @@ -39,6 +41,8 @@
>>> struct AspeedINTCClass {
>>>
>>> uint32_t num_lines;
>>> uint32_t num_ints;
>>> + uint64_t mem_size;
>>> + uint64_t reg_size;
>>> };
>>>
>>> #endif /* ASPEED_INTC_H */
>
Hi Cedric,
> >> and the register array as:
> >>
> >> uint32_t regs[ASPEED_INTC_NR_REGS];
> >>
> >> The number of regs looks pretty big for me. Are the registers
> >> covering the whole MMIO aperture ?
> >>
> > According to the datasheet, the entire register address space size of
> > INTC (CPU DIE) is 16KB (0x12100000-0x12103FFF). Therefore, I set the
> memory size to 0x4000.
> > Currently, we need to use the "GICINT192-201 raw status and clear" register
> INTC1B04.
> > Thus, an array size of 0x2000 is sufficient.
>
> yes but we are only using these regs :
>
> 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)
> REG32(GICINT192_201_EN, 0x1B00)
> REG32(GICINT192_201_STATUS, 0x1B04)
>
> so the first 0x1000 are unused.
>
>
> >
> > However, we are going to increase the size to 0x3000 to support the
> > co-processors SSP and TSP In the another patch series.
> > We also need to include the following register definitions:
> >
> > /* SSP INTC Registers */
> > REG32(SSPINT128_EN, 0x2000)
> > REG32(SSPINT128_STATUS, 0x2004)
> > REG32(SSPINT129_EN, 0x2100)
> > REG32(SSPINT129_STATUS, 0x2104)
> > REG32(SSPINT130_EN, 0x2200)
> > REG32(SSPINT130_STATUS, 0x2204)
> > REG32(SSPINT131_EN, 0x2300)
> > REG32(SSPINT131_STATUS, 0x2304)
> > REG32(SSPINT132_EN, 0x2400)
> > REG32(SSPINT132_STATUS, 0x2404)
> > REG32(SSPINT133_EN, 0x2500)
> > REG32(SSPINT133_STATUS, 0x2504)
> > REG32(SSPINT134_EN, 0x2600)
> > REG32(SSPINT134_STATUS, 0x2604)
> > REG32(SSPINT135_EN, 0x2700)
> > REG32(SSPINT135_STATUS, 0x2704)
> > REG32(SSPINT136_EN, 0x2800)
> > REG32(SSPINT136_STATUS, 0x2804)
> > REG32(SSPINT137_EN, 0x2900)
> > REG32(SSPINT137_STATUS, 0x2904)
> > REG32(SSPINT138_EN, 0x2A00)
> > REG32(SSPINT138_STATUS, 0x2A04)
> > REG32(SSPINT160_169_EN, 0x2B00)
> > REG32(SSPINT160_169_STATUS, 0x2B04)
> >
> >>
> >>> + if (offset >= aic->reg_size) {
> >>
> >> This is dead code since the MMIO aperture has the same size. You
> >> could remove the check.
> >
> > Will remove.
> >>
> >>> qemu_log_mask(LOG_GUEST_ERROR,
> >>> "%s: Out-of-bounds read at offset 0x%"
> >> HWADDR_PRIx "\n",
> >>> __func__, offset); @@ -143,7 +144,7 @@
> >> static
> >>> void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
> >>> uint32_t change;
> >>> uint32_t irq;
> >>>
> >>> - if (addr >= ASPEED_INTC_NR_REGS) {
> >>> + if (offset >= aic->reg_size) {
> >>> qemu_log_mask(LOG_GUEST_ERROR,
> >>> "%s: Out-of-bounds write at offset 0x%"
> >> HWADDR_PRIx "\n",
> >>> __func__, offset); @@ -302,10 +303,16
> @@
> >>> static void aspeed_intc_realize(DeviceState *dev, Error **errp)
> >>> AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> >>> int i;
> >>>
> >>> + memory_region_init(&s->iomem_container, OBJECT(s),
> >>> + TYPE_ASPEED_INTC ".container", aic->mem_size);
> >>> +
> >>> + sysbus_init_mmio(sbd, &s->iomem_container);
> >>
> >> Why introduce a container ? Do you plan to have multiple sub-regions ?
> >>
> > I created a container to save the entire register address space of
> > INTC and its sub-region to place the actual used register address space.
> > 0000000012100000-0000000012103fff (prio 0, i/o): aspeed.intc.container
> > 0000000012100000-0000000012101fff (prio 0, i/o):
> > aspeed.intc.regs 0000000014c18000-0000000014c183ff (prio 0, i/o):
> aspeed.intc.container
> > 0000000014c18000-0000000014c183d7 (prio 0, i/o):
> > aspeed.intc.regs
> >
> > If I misunderstood this design, I will change it to have two memory regions
> for INTC and INTCIO, respectively.
> > If need, I will change to the following memory regions. --> it removes
> containers.
> > 0000000012100000-0000000012101fff (prio 0, i/o):
> aspeed.intc.regs
> > 0000000014c18000-0000000014c183d7 (prio 0, i/o):
> > aspeed.intc.regs
>
> I don't think the region container is useful in that case since there is only a
> single region per model.
>
> However, we could "optimize" a bit the MMIO apertures to avoid mapping
> huge unused gaps and only map the useful set of registers :
>
> INTC Registers [ 0x1000 - 0x1B04 ]
> SSP INTC Registers [ 0x2000 - 0x2B04 ]
> INTCIO Registers [ 0x0100 - 0x0154 ]
>
> Each set would be associated with a subregion which would be mapped at the
> right offset in the region container.
>
> This is just a suggestion.
>
I followed your suggestion to reduce the size of the register array. Here's what I've done:
0000000012100000-0000000012103fff (prio 0, i/o): aspeed.intc.container
0000000012101000-0000000012101b07 (prio 0, i/o): aspeed.intc.regs
A solution:
1. I created an INTC Register sub-region mapped at offset 0x1000 with a size of 0xb08.
This change adjusted the following register offsets to their original values minus 0x1000:
REG32(GICINT128_EN, 0x000)
REG32(GICINT128_STATUS, 0x004)
REG32(GICINT129_EN, 0x100)
REG32(GICINT129_STATUS, 0x104)
REG32(GICINT130_EN, 0x200)
REG32(GICINT130_STATUS, 0x204)
REG32(GICINT131_EN, 0x300)
REG32(GICINT131_STATUS, 0x304)
REG32(GICINT132_EN, 0x400)
REG32(GICINT132_STATUS, 0x404)
REG32(GICINT133_EN, 0x500)
REG32(GICINT133_STATUS, 0x504)
REG32(GICINT134_EN, 0x600)
REG32(GICINT134_STATUS, 0x604)
REG32(GICINT135_EN, 0x700)
REG32(GICINT135_STATUS, 0x704)
REG32(GICINT136_EN, 0x800)
REG32(GICINT136_STATUS, 0x804)
REG32(GICINT192_201_EN, 0xB00)
REG32(GICINT192_201_STATUS, 0xB04)
2. To support multiple sub-regions, such as for SSP, I created a new sub-region mapped at offset 0x2000.
This adjustment changed the register offsets to their original values minus 0x2000:
/* SSP INTC Registers */
> REG32(SSPINT128_EN, 0x000)
> REG32(SSPINT128_STATUS, 0x004)
> REG32(SSPINT129_EN, 0x100)
> REG32(SSPINT129_STATUS, 0x104)
> REG32(SSPINT130_EN, 0x200)
> REG32(SSPINT130_STATUS, 0x204)
> REG32(SSPINT131_EN, 0x300)
> REG32(SSPINT131_STATUS, 0x304)
> REG32(SSPINT132_EN, 0x400)
> REG32(SSPINT132_STATUS, 0x404)
> REG32(SSPINT133_EN, 0x500)
> REG32(SSPINT133_STATUS, 0x504)
> REG32(SSPINT134_EN, 0x600)
> REG32(SSPINT134_STATUS, 0x604)
> REG32(SSPINT135_EN, 0x700)
> REG32(SSPINT135_STATUS, 0x704)
> REG32(SSPINT136_EN, 0x800)
> REG32(SSPINT136_STATUS, 0x804)
> REG32(SSPINT137_EN, 0x900)
> REG32(SSPINT137_STATUS, 0x904)
> REG32(SSPINT138_EN, 0xA00)
> REG32(SSPINT138_STATUS, 0xA04)
> REG32(SSPINT160_169_EN, 0xB00)
> REG32(SSPINT160_169_STATUS, 0xB04)
Regarding handling identical offsets like SSPINT128_EN and GICINT128_EN, which both use offset 0x00,
they currently share the same aspeed_intc_ops and IRQ handler functions like aspeed_intc_status_handler.
How can I check whether the offset belongs to the SSP or INTC region?
Can I create new aspeed_intc_ssp_write and aspeed_intc_ssp_read callback functions specifically for
reading and writing to the SSP region, and then add new parameters to the IRQ handler to distinguish
between the SSP and INTC regions?
B solution:
There is another solution where we don't consider multiple sub-regions and only support a single sub-region.
Since the SSP runs on a co-processor, we've created a new SSP class:
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_intcio_info);
+ type_register_static(&aspeed_2700_ssp_intc_info);
+ type_register_static(&aspeed_2700_ssp_intcio_info);
}
In this solution, we don’t need to make many changes because the SSP has its own AspeedINTCState register array.
As a result, most functions can be reused to support SSP.
The SSP uses the same callback function as the INTC:
static void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
unsigned size)
{
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_GICINT192_201_EN:
+ case R_SSPINT128_STATUS:
+ case R_SSPINT129_STATUS:
+ case R_SSPINT130_STATUS:
Since both GICINT128_EN and SSPINT128_EN share the same offset of 0, I will modify the code to handle INT128 for both.
Could you please give me any suggestion?
Thanks-Jamin
>
> Thanks,
>
> C.
>
>
>
> > Do you expect the above memory regions for INTC?
> >
> > Thanks for your review and suggestion.
> > Jamin
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >>
> >>
> >>> +
> >>> memory_region_init_io(&s->iomem, OBJECT(s),
> &aspeed_intc_ops,
> >> s,
> >>> - TYPE_ASPEED_INTC ".regs",
> >> ASPEED_INTC_NR_REGS << 2);
> >>> + TYPE_ASPEED_INTC ".regs",
> aic->reg_size);
> >>> +
> >>> + memory_region_add_subregion(&s->iomem_container, 0x0,
> >> &s->iomem);
> >>>
> >>> - sysbus_init_mmio(sbd, &s->iomem);
> >>> qdev_init_gpio_in(dev, aspeed_intc_set_irq, aic->num_ints);
> >>>
> >>> for (i = 0; i < aic->num_ints; i++) { @@ -344,6 +351,8 @@
> >>> static void aspeed_2700_intc_class_init(ObjectClass *klass, void *data)
> >>> dc->desc = "ASPEED 2700 INTC Controller";
> >>> aic->num_lines = 32;
> >>> aic->num_ints = 9;
> >>> + aic->mem_size = 0x4000;
> >>> + aic->reg_size = 0x2000;
> >>> }
> >>>
> >>> static const TypeInfo aspeed_2700_intc_info = { diff --git
> >>> a/include/hw/intc/aspeed_intc.h b/include/hw/intc/aspeed_intc.h
> >>> index 18cb43476c..ecaeb15aea 100644
> >>> --- a/include/hw/intc/aspeed_intc.h
> >>> +++ b/include/hw/intc/aspeed_intc.h
> >>> @@ -25,6 +25,8 @@ struct AspeedINTCState {
> >>>
> >>> /*< public >*/
> >>> MemoryRegion iomem;
> >>> + MemoryRegion iomem_container;
> >>> +
> >>> uint32_t regs[ASPEED_INTC_NR_REGS];
> >>> OrIRQState orgates[ASPEED_INTC_NR_INTS];
> >>> qemu_irq output_pins[ASPEED_INTC_NR_INTS]; @@ -39,6 +41,8
> @@
> >>> struct AspeedINTCClass {
> >>>
> >>> uint32_t num_lines;
> >>> uint32_t num_ints;
> >>> + uint64_t mem_size;
> >>> + uint64_t reg_size;
> >>> };
> >>>
> >>> #endif /* ASPEED_INTC_H */
> >
On 2/26/25 04:40, Jamin Lin wrote:
> Hi Cedric,
>
>>>> and the register array as:
>>>>
>>>> uint32_t regs[ASPEED_INTC_NR_REGS];
>>>>
>>>> The number of regs looks pretty big for me. Are the registers
>>>> covering the whole MMIO aperture ?
>>>>
>>> According to the datasheet, the entire register address space size of
>>> INTC (CPU DIE) is 16KB (0x12100000-0x12103FFF). Therefore, I set the
>> memory size to 0x4000.
>>> Currently, we need to use the "GICINT192-201 raw status and clear" register
>> INTC1B04.
>>> Thus, an array size of 0x2000 is sufficient.
>>
>> yes but we are only using these regs :
>>
>> 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)
>> REG32(GICINT192_201_EN, 0x1B00)
>> REG32(GICINT192_201_STATUS, 0x1B04)
>>
>> so the first 0x1000 are unused.
>>
>>
>>>
>>> However, we are going to increase the size to 0x3000 to support the
>>> co-processors SSP and TSP In the another patch series.
>>> We also need to include the following register definitions:
>>>
>>> /* SSP INTC Registers */
>>> REG32(SSPINT128_EN, 0x2000)
>>> REG32(SSPINT128_STATUS, 0x2004)
>>> REG32(SSPINT129_EN, 0x2100)
>>> REG32(SSPINT129_STATUS, 0x2104)
>>> REG32(SSPINT130_EN, 0x2200)
>>> REG32(SSPINT130_STATUS, 0x2204)
>>> REG32(SSPINT131_EN, 0x2300)
>>> REG32(SSPINT131_STATUS, 0x2304)
>>> REG32(SSPINT132_EN, 0x2400)
>>> REG32(SSPINT132_STATUS, 0x2404)
>>> REG32(SSPINT133_EN, 0x2500)
>>> REG32(SSPINT133_STATUS, 0x2504)
>>> REG32(SSPINT134_EN, 0x2600)
>>> REG32(SSPINT134_STATUS, 0x2604)
>>> REG32(SSPINT135_EN, 0x2700)
>>> REG32(SSPINT135_STATUS, 0x2704)
>>> REG32(SSPINT136_EN, 0x2800)
>>> REG32(SSPINT136_STATUS, 0x2804)
>>> REG32(SSPINT137_EN, 0x2900)
>>> REG32(SSPINT137_STATUS, 0x2904)
>>> REG32(SSPINT138_EN, 0x2A00)
>>> REG32(SSPINT138_STATUS, 0x2A04)
>>> REG32(SSPINT160_169_EN, 0x2B00)
>>> REG32(SSPINT160_169_STATUS, 0x2B04)
>>>
>>>>
>>>>> + if (offset >= aic->reg_size) {
>>>>
>>>> This is dead code since the MMIO aperture has the same size. You
>>>> could remove the check.
>>>
>>> Will remove.
>>>>
>>>>> qemu_log_mask(LOG_GUEST_ERROR,
>>>>> "%s: Out-of-bounds read at offset 0x%"
>>>> HWADDR_PRIx "\n",
>>>>> __func__, offset); @@ -143,7 +144,7 @@
>>>> static
>>>>> void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
>>>>> uint32_t change;
>>>>> uint32_t irq;
>>>>>
>>>>> - if (addr >= ASPEED_INTC_NR_REGS) {
>>>>> + if (offset >= aic->reg_size) {
>>>>> qemu_log_mask(LOG_GUEST_ERROR,
>>>>> "%s: Out-of-bounds write at offset 0x%"
>>>> HWADDR_PRIx "\n",
>>>>> __func__, offset); @@ -302,10 +303,16
>> @@
>>>>> static void aspeed_intc_realize(DeviceState *dev, Error **errp)
>>>>> AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
>>>>> int i;
>>>>>
>>>>> + memory_region_init(&s->iomem_container, OBJECT(s),
>>>>> + TYPE_ASPEED_INTC ".container", aic->mem_size);
>>>>> +
>>>>> + sysbus_init_mmio(sbd, &s->iomem_container);
>>>>
>>>> Why introduce a container ? Do you plan to have multiple sub-regions ?
>>>>
>>> I created a container to save the entire register address space of
>>> INTC and its sub-region to place the actual used register address space.
>>> 0000000012100000-0000000012103fff (prio 0, i/o): aspeed.intc.container
>>> 0000000012100000-0000000012101fff (prio 0, i/o):
>>> aspeed.intc.regs 0000000014c18000-0000000014c183ff (prio 0, i/o):
>> aspeed.intc.container
>>> 0000000014c18000-0000000014c183d7 (prio 0, i/o):
>>> aspeed.intc.regs
>>>
>>> If I misunderstood this design, I will change it to have two memory regions
>> for INTC and INTCIO, respectively.
>>> If need, I will change to the following memory regions. --> it removes
>> containers.
>>> 0000000012100000-0000000012101fff (prio 0, i/o):
>> aspeed.intc.regs
>>> 0000000014c18000-0000000014c183d7 (prio 0, i/o):
>>> aspeed.intc.regs
>>
>> I don't think the region container is useful in that case since there is only a
>> single region per model.
>>
>> However, we could "optimize" a bit the MMIO apertures to avoid mapping
>> huge unused gaps and only map the useful set of registers :
>>
>> INTC Registers [ 0x1000 - 0x1B04 ]
>> SSP INTC Registers [ 0x2000 - 0x2B04 ]
>> INTCIO Registers [ 0x0100 - 0x0154 ]
>>
>> Each set would be associated with a subregion which would be mapped at the
>> right offset in the region container.
>>
>> This is just a suggestion.
>>
>
> I followed your suggestion to reduce the size of the register array. Here's what I've done:
>
> 0000000012100000-0000000012103fff (prio 0, i/o): aspeed.intc.container
> 0000000012101000-0000000012101b07 (prio 0, i/o): aspeed.intc.regs
>
> A solution:
> 1. I created an INTC Register sub-region mapped at offset 0x1000 with a size of 0xb08.
> This change adjusted the following register offsets to their original values minus 0x1000:
>
> REG32(GICINT128_EN, 0x000)
> REG32(GICINT128_STATUS, 0x004)
> REG32(GICINT129_EN, 0x100)
> REG32(GICINT129_STATUS, 0x104)
> REG32(GICINT130_EN, 0x200)
> REG32(GICINT130_STATUS, 0x204)
> REG32(GICINT131_EN, 0x300)
> REG32(GICINT131_STATUS, 0x304)
> REG32(GICINT132_EN, 0x400)
> REG32(GICINT132_STATUS, 0x404)
> REG32(GICINT133_EN, 0x500)
> REG32(GICINT133_STATUS, 0x504)
> REG32(GICINT134_EN, 0x600)
> REG32(GICINT134_STATUS, 0x604)
> REG32(GICINT135_EN, 0x700)
> REG32(GICINT135_STATUS, 0x704)
> REG32(GICINT136_EN, 0x800)
> REG32(GICINT136_STATUS, 0x804)
> REG32(GICINT192_201_EN, 0xB00)
> REG32(GICINT192_201_STATUS, 0xB04)
>
> 2. To support multiple sub-regions, such as for SSP, I created a new sub-region mapped at offset 0x2000.
> This adjustment changed the register offsets to their original values minus 0x2000:
>
> /* SSP INTC Registers */
>> REG32(SSPINT128_EN, 0x000)
>> REG32(SSPINT128_STATUS, 0x004)
>> REG32(SSPINT129_EN, 0x100)
>> REG32(SSPINT129_STATUS, 0x104)
>> REG32(SSPINT130_EN, 0x200)
>> REG32(SSPINT130_STATUS, 0x204)
>> REG32(SSPINT131_EN, 0x300)
>> REG32(SSPINT131_STATUS, 0x304)
>> REG32(SSPINT132_EN, 0x400)
>> REG32(SSPINT132_STATUS, 0x404)
>> REG32(SSPINT133_EN, 0x500)
>> REG32(SSPINT133_STATUS, 0x504)
>> REG32(SSPINT134_EN, 0x600)
>> REG32(SSPINT134_STATUS, 0x604)
>> REG32(SSPINT135_EN, 0x700)
>> REG32(SSPINT135_STATUS, 0x704)
>> REG32(SSPINT136_EN, 0x800)
>> REG32(SSPINT136_STATUS, 0x804)
>> REG32(SSPINT137_EN, 0x900)
>> REG32(SSPINT137_STATUS, 0x904)
>> REG32(SSPINT138_EN, 0xA00)
>> REG32(SSPINT138_STATUS, 0xA04)
>> REG32(SSPINT160_169_EN, 0xB00)
>> REG32(SSPINT160_169_STATUS, 0xB04)
>
> Regarding handling identical offsets like SSPINT128_EN and GICINT128_EN, which both use offset 0x00,
> they currently share the same aspeed_intc_ops and IRQ handler functions like aspeed_intc_status_handler.
You need a MemoryRegionOps per region.
> How can I check whether the offset belongs to the SSP or INTC region?
> Can I create new aspeed_intc_ssp_write and aspeed_intc_ssp_read callback functions specifically for
> reading and writing to the SSP region, and then add new parameters to the IRQ handler to distinguish
> between the SSP and INTC regions?
I haven't looked at that part yet.
>
> B solution:
>
> There is another solution where we don't consider multiple sub-regions and only support a single sub-region.
> Since the SSP runs on a co-processor, we've created a new SSP class:
> 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_intcio_info);
> + type_register_static(&aspeed_2700_ssp_intc_info);
> + type_register_static(&aspeed_2700_ssp_intcio_info);
> }
>
> In this solution, we don’t need to make many changes because the SSP has its own AspeedINTCState register array.
> As a result, most functions can be reused to support SSP.
> The SSP uses the same callback function as the INTC:
>
> static void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
> unsigned size)
> {
> 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_GICINT192_201_EN:
> + case R_SSPINT128_STATUS:
> + case R_SSPINT129_STATUS:
> + case R_SSPINT130_STATUS:
>
> Since both GICINT128_EN and SSPINT128_EN share the same offset of 0, I will modify the code to handle INT128 for both.
>
> Could you please give me any suggestion?
Please send a series.
As a reminder, soft-freeze is in less than 2 weeks. Please try to organize
the current series so that the more obvious ones can be merged easily.
Thanks,
C.
>
> Thanks-Jamin
>
>>
>> Thanks,
>>
>> C.
>>
>>
>>
>>> Do you expect the above memory regions for INTC?
>>>
>>> Thanks for your review and suggestion.
>>> Jamin
>>>>
>>>> Thanks,
>>>>
>>>> C.
>>>>
>>>>
>>>>
>>>>> +
>>>>> memory_region_init_io(&s->iomem, OBJECT(s),
>> &aspeed_intc_ops,
>>>> s,
>>>>> - TYPE_ASPEED_INTC ".regs",
>>>> ASPEED_INTC_NR_REGS << 2);
>>>>> + TYPE_ASPEED_INTC ".regs",
>> aic->reg_size);
>>>>> +
>>>>> + memory_region_add_subregion(&s->iomem_container, 0x0,
>>>> &s->iomem);
>>>>>
>>>>> - sysbus_init_mmio(sbd, &s->iomem);
>>>>> qdev_init_gpio_in(dev, aspeed_intc_set_irq, aic->num_ints);
>>>>>
>>>>> for (i = 0; i < aic->num_ints; i++) { @@ -344,6 +351,8 @@
>>>>> static void aspeed_2700_intc_class_init(ObjectClass *klass, void *data)
>>>>> dc->desc = "ASPEED 2700 INTC Controller";
>>>>> aic->num_lines = 32;
>>>>> aic->num_ints = 9;
>>>>> + aic->mem_size = 0x4000;
>>>>> + aic->reg_size = 0x2000;
>>>>> }
>>>>>
>>>>> static const TypeInfo aspeed_2700_intc_info = { diff --git
>>>>> a/include/hw/intc/aspeed_intc.h b/include/hw/intc/aspeed_intc.h
>>>>> index 18cb43476c..ecaeb15aea 100644
>>>>> --- a/include/hw/intc/aspeed_intc.h
>>>>> +++ b/include/hw/intc/aspeed_intc.h
>>>>> @@ -25,6 +25,8 @@ struct AspeedINTCState {
>>>>>
>>>>> /*< public >*/
>>>>> MemoryRegion iomem;
>>>>> + MemoryRegion iomem_container;
>>>>> +
>>>>> uint32_t regs[ASPEED_INTC_NR_REGS];
>>>>> OrIRQState orgates[ASPEED_INTC_NR_INTS];
>>>>> qemu_irq output_pins[ASPEED_INTC_NR_INTS]; @@ -39,6 +41,8
>> @@
>>>>> struct AspeedINTCClass {
>>>>>
>>>>> uint32_t num_lines;
>>>>> uint32_t num_ints;
>>>>> + uint64_t mem_size;
>>>>> + uint64_t reg_size;
>>>>> };
>>>>>
>>>>> #endif /* ASPEED_INTC_H */
>>>
>
Hi Cedric,
> Cc: Troy Lee <troy_lee@aspeedtech.com>
> Subject: Re: [PATCH v3 01/28] hw/intc/aspeed: Support setting different
> memory and register size
>
> On 2/26/25 04:40, Jamin Lin wrote:
> > Hi Cedric,
> >
> >>>> and the register array as:
> >>>>
> >>>> uint32_t regs[ASPEED_INTC_NR_REGS];
> >>>>
> >>>> The number of regs looks pretty big for me. Are the registers
> >>>> covering the whole MMIO aperture ?
> >>>>
> >>> According to the datasheet, the entire register address space size
> >>> of INTC (CPU DIE) is 16KB (0x12100000-0x12103FFF). Therefore, I set
> >>> the
> >> memory size to 0x4000.
> >>> Currently, we need to use the "GICINT192-201 raw status and clear"
> >>> register
> >> INTC1B04.
> >>> Thus, an array size of 0x2000 is sufficient.
> >>
> >> yes but we are only using these regs :
> >>
> >> 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)
> >> REG32(GICINT192_201_EN, 0x1B00)
> >> REG32(GICINT192_201_STATUS, 0x1B04)
> >>
> >> so the first 0x1000 are unused.
> >>
> >>
> >>>
> >>> However, we are going to increase the size to 0x3000 to support the
> >>> co-processors SSP and TSP In the another patch series.
> >>> We also need to include the following register definitions:
> >>>
> >>> /* SSP INTC Registers */
> >>> REG32(SSPINT128_EN, 0x2000)
> >>> REG32(SSPINT128_STATUS, 0x2004)
> >>> REG32(SSPINT129_EN, 0x2100)
> >>> REG32(SSPINT129_STATUS, 0x2104)
> >>> REG32(SSPINT130_EN, 0x2200)
> >>> REG32(SSPINT130_STATUS, 0x2204)
> >>> REG32(SSPINT131_EN, 0x2300)
> >>> REG32(SSPINT131_STATUS, 0x2304)
> >>> REG32(SSPINT132_EN, 0x2400)
> >>> REG32(SSPINT132_STATUS, 0x2404)
> >>> REG32(SSPINT133_EN, 0x2500)
> >>> REG32(SSPINT133_STATUS, 0x2504)
> >>> REG32(SSPINT134_EN, 0x2600)
> >>> REG32(SSPINT134_STATUS, 0x2604)
> >>> REG32(SSPINT135_EN, 0x2700)
> >>> REG32(SSPINT135_STATUS, 0x2704)
> >>> REG32(SSPINT136_EN, 0x2800)
> >>> REG32(SSPINT136_STATUS, 0x2804)
> >>> REG32(SSPINT137_EN, 0x2900)
> >>> REG32(SSPINT137_STATUS, 0x2904)
> >>> REG32(SSPINT138_EN, 0x2A00)
> >>> REG32(SSPINT138_STATUS, 0x2A04)
> >>> REG32(SSPINT160_169_EN, 0x2B00)
> >>> REG32(SSPINT160_169_STATUS, 0x2B04)
> >>>
> >>>>
> >>>>> + if (offset >= aic->reg_size) {
> >>>>
> >>>> This is dead code since the MMIO aperture has the same size. You
> >>>> could remove the check.
> >>>
> >>> Will remove.
> >>>>
> >>>>> qemu_log_mask(LOG_GUEST_ERROR,
> >>>>> "%s: Out-of-bounds read at offset 0x%"
> >>>> HWADDR_PRIx "\n",
> >>>>> __func__, offset); @@ -143,7 +144,7
> @@
> >>>> static
> >>>>> void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
> >>>>> uint32_t change;
> >>>>> uint32_t irq;
> >>>>>
> >>>>> - if (addr >= ASPEED_INTC_NR_REGS) {
> >>>>> + if (offset >= aic->reg_size) {
> >>>>> qemu_log_mask(LOG_GUEST_ERROR,
> >>>>> "%s: Out-of-bounds write at offset 0x%"
> >>>> HWADDR_PRIx "\n",
> >>>>> __func__, offset); @@ -302,10 +303,16
> >> @@
> >>>>> static void aspeed_intc_realize(DeviceState *dev, Error **errp)
> >>>>> AspeedINTCClass *aic = ASPEED_INTC_GET_CLASS(s);
> >>>>> int i;
> >>>>>
> >>>>> + memory_region_init(&s->iomem_container, OBJECT(s),
> >>>>> + TYPE_ASPEED_INTC ".container", aic->mem_size);
> >>>>> +
> >>>>> + sysbus_init_mmio(sbd, &s->iomem_container);
> >>>>
> >>>> Why introduce a container ? Do you plan to have multiple sub-regions ?
> >>>>
> >>> I created a container to save the entire register address space of
> >>> INTC and its sub-region to place the actual used register address space.
> >>> 0000000012100000-0000000012103fff (prio 0, i/o): aspeed.intc.container
> >>> 0000000012100000-0000000012101fff (prio 0, i/o):
> >>> aspeed.intc.regs 0000000014c18000-0000000014c183ff (prio 0, i/o):
> >> aspeed.intc.container
> >>> 0000000014c18000-0000000014c183d7 (prio 0, i/o):
> >>> aspeed.intc.regs
> >>>
> >>> If I misunderstood this design, I will change it to have two memory
> >>> regions
> >> for INTC and INTCIO, respectively.
> >>> If need, I will change to the following memory regions. --> it
> >>> removes
> >> containers.
> >>> 0000000012100000-0000000012101fff (prio 0, i/o):
> >> aspeed.intc.regs
> >>> 0000000014c18000-0000000014c183d7 (prio 0, i/o):
> >>> aspeed.intc.regs
> >>
> >> I don't think the region container is useful in that case since there
> >> is only a single region per model.
> >>
> >> However, we could "optimize" a bit the MMIO apertures to avoid
> >> mapping huge unused gaps and only map the useful set of registers :
> >>
> >> INTC Registers [ 0x1000 - 0x1B04 ]
> >> SSP INTC Registers [ 0x2000 - 0x2B04 ]
> >> INTCIO Registers [ 0x0100 - 0x0154 ]
> >>
> >> Each set would be associated with a subregion which would be mapped
> >> at the right offset in the region container.
> >>
> >> This is just a suggestion.
> >>
> >
> > I followed your suggestion to reduce the size of the register array. Here's
> what I've done:
> >
> > 0000000012100000-0000000012103fff (prio 0, i/o): aspeed.intc.container
> > 0000000012101000-0000000012101b07 (prio 0, i/o):
> > aspeed.intc.regs
> >
> > A solution:
> > 1. I created an INTC Register sub-region mapped at offset 0x1000 with a size
> of 0xb08.
> > This change adjusted the following register offsets to their original values
> minus 0x1000:
> >
> > REG32(GICINT128_EN, 0x000)
> > REG32(GICINT128_STATUS, 0x004)
> > REG32(GICINT129_EN, 0x100)
> > REG32(GICINT129_STATUS, 0x104)
> > REG32(GICINT130_EN, 0x200)
> > REG32(GICINT130_STATUS, 0x204)
> > REG32(GICINT131_EN, 0x300)
> > REG32(GICINT131_STATUS, 0x304)
> > REG32(GICINT132_EN, 0x400)
> > REG32(GICINT132_STATUS, 0x404)
> > REG32(GICINT133_EN, 0x500)
> > REG32(GICINT133_STATUS, 0x504)
> > REG32(GICINT134_EN, 0x600)
> > REG32(GICINT134_STATUS, 0x604)
> > REG32(GICINT135_EN, 0x700)
> > REG32(GICINT135_STATUS, 0x704)
> > REG32(GICINT136_EN, 0x800)
> > REG32(GICINT136_STATUS, 0x804)
> > REG32(GICINT192_201_EN, 0xB00)
> > REG32(GICINT192_201_STATUS, 0xB04)
> >
> > 2. To support multiple sub-regions, such as for SSP, I created a new
> sub-region mapped at offset 0x2000.
> > This adjustment changed the register offsets to their original values minus
> 0x2000:
> >
> > /* SSP INTC Registers */
> >> REG32(SSPINT128_EN, 0x000)
> >> REG32(SSPINT128_STATUS, 0x004)
> >> REG32(SSPINT129_EN, 0x100)
> >> REG32(SSPINT129_STATUS, 0x104)
> >> REG32(SSPINT130_EN, 0x200)
> >> REG32(SSPINT130_STATUS, 0x204)
> >> REG32(SSPINT131_EN, 0x300)
> >> REG32(SSPINT131_STATUS, 0x304)
> >> REG32(SSPINT132_EN, 0x400)
> >> REG32(SSPINT132_STATUS, 0x404)
> >> REG32(SSPINT133_EN, 0x500)
> >> REG32(SSPINT133_STATUS, 0x504)
> >> REG32(SSPINT134_EN, 0x600)
> >> REG32(SSPINT134_STATUS, 0x604)
> >> REG32(SSPINT135_EN, 0x700)
> >> REG32(SSPINT135_STATUS, 0x704)
> >> REG32(SSPINT136_EN, 0x800)
> >> REG32(SSPINT136_STATUS, 0x804)
> >> REG32(SSPINT137_EN, 0x900)
> >> REG32(SSPINT137_STATUS, 0x904)
> >> REG32(SSPINT138_EN, 0xA00)
> >> REG32(SSPINT138_STATUS, 0xA04)
> >> REG32(SSPINT160_169_EN, 0xB00)
> >> REG32(SSPINT160_169_STATUS, 0xB04)
> >
> > Regarding handling identical offsets like SSPINT128_EN and
> > GICINT128_EN, which both use offset 0x00, they currently share the same
> aspeed_intc_ops and IRQ handler functions like aspeed_intc_status_handler.
>
> You need a MemoryRegionOps per region.
>
Got it. Thanks for your suggestion.
> > How can I check whether the offset belongs to the SSP or INTC region?
> > Can I create new aspeed_intc_ssp_write and aspeed_intc_ssp_read
> > callback functions specifically for reading and writing to the SSP
> > region, and then add new parameters to the IRQ handler to distinguish
> between the SSP and INTC regions?
>
> I haven't looked at that part yet.
>
> >
> > B solution:
> >
> > There is another solution where we don't consider multiple sub-regions and
> only support a single sub-region.
> > Since the SSP runs on a co-processor, we've created a new SSP class:
> > 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_intcio_info);
> > + type_register_static(&aspeed_2700_ssp_intc_info);
> > + type_register_static(&aspeed_2700_ssp_intcio_info);
> > }
> >
> > In this solution, we don’t need to make many changes because the SSP has
> its own AspeedINTCState register array.
> > As a result, most functions can be reused to support SSP.
> > The SSP uses the same callback function as the INTC:
> >
> > static void aspeed_intc_write(void *opaque, hwaddr offset, uint64_t data,
> > unsigned size) {
> > 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_GICINT192_201_EN:
> > + case R_SSPINT128_STATUS:
> > + case R_SSPINT129_STATUS:
> > + case R_SSPINT130_STATUS:
> >
> > Since both GICINT128_EN and SSPINT128_EN share the same offset of 0, I
> will modify the code to handle INT128 for both.
> >
> > Could you please give me any suggestion?
>
> Please send a series.
>
> As a reminder, soft-freeze is in less than 2 weeks. Please try to organize the
> current series so that the more obvious ones can be merged easily.
>
Got it.
I will send patch as soon as possible.
Jamin
>
> Thanks,
>
> C.
>
>
> >
> > Thanks-Jamin
> >
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >>
> >>
> >>> Do you expect the above memory regions for INTC?
> >>>
> >>> Thanks for your review and suggestion.
> >>> Jamin
> >>>>
> >>>> Thanks,
> >>>>
> >>>> C.
> >>>>
> >>>>
> >>>>
> >>>>> +
> >>>>> memory_region_init_io(&s->iomem, OBJECT(s),
> >> &aspeed_intc_ops,
> >>>> s,
> >>>>> - TYPE_ASPEED_INTC ".regs",
> >>>> ASPEED_INTC_NR_REGS << 2);
> >>>>> + TYPE_ASPEED_INTC ".regs",
> >> aic->reg_size);
> >>>>> +
> >>>>> + memory_region_add_subregion(&s->iomem_container, 0x0,
> >>>> &s->iomem);
> >>>>>
> >>>>> - sysbus_init_mmio(sbd, &s->iomem);
> >>>>> qdev_init_gpio_in(dev, aspeed_intc_set_irq,
> >>>>> aic->num_ints);
> >>>>>
> >>>>> for (i = 0; i < aic->num_ints; i++) { @@ -344,6 +351,8 @@
> >>>>> static void aspeed_2700_intc_class_init(ObjectClass *klass, void *data)
> >>>>> dc->desc = "ASPEED 2700 INTC Controller";
> >>>>> aic->num_lines = 32;
> >>>>> aic->num_ints = 9;
> >>>>> + aic->mem_size = 0x4000;
> >>>>> + aic->reg_size = 0x2000;
> >>>>> }
> >>>>>
> >>>>> static const TypeInfo aspeed_2700_intc_info = { diff --git
> >>>>> a/include/hw/intc/aspeed_intc.h b/include/hw/intc/aspeed_intc.h
> >>>>> index 18cb43476c..ecaeb15aea 100644
> >>>>> --- a/include/hw/intc/aspeed_intc.h
> >>>>> +++ b/include/hw/intc/aspeed_intc.h
> >>>>> @@ -25,6 +25,8 @@ struct AspeedINTCState {
> >>>>>
> >>>>> /*< public >*/
> >>>>> MemoryRegion iomem;
> >>>>> + MemoryRegion iomem_container;
> >>>>> +
> >>>>> uint32_t regs[ASPEED_INTC_NR_REGS];
> >>>>> OrIRQState orgates[ASPEED_INTC_NR_INTS];
> >>>>> qemu_irq output_pins[ASPEED_INTC_NR_INTS]; @@ -39,6
> +41,8
> >> @@
> >>>>> struct AspeedINTCClass {
> >>>>>
> >>>>> uint32_t num_lines;
> >>>>> uint32_t num_ints;
> >>>>> + uint64_t mem_size;
> >>>>> + uint64_t reg_size;
> >>>>> };
> >>>>>
> >>>>> #endif /* ASPEED_INTC_H */
> >>>
> >
© 2016 - 2026 Red Hat, Inc.