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_...@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 */
