Hi Cedric,
> Subject: Re: [PATCH v4 03/23] hw/intc/aspeed: Reduce regs array size by
> adding a register sub-region
>
> On 3/3/25 10:54, Jamin Lin wrote:
> > Currently, the size of the "regs" array is 0x2000, which is too large.
> > So far, it only uses "GICINT128 to `GICINT134", and the offsets from 0
> > to 0x1000 are unused. To save code size and avoid mapping large unused
> > gaps, update to only map the useful set of registers:
> >
> > INTC register [0x1000 – 0x1804]
> >
> > Update "reg_size" to 0x808. Introduce a new class attribute
> > "reg_offset" to set the start offset of a "INTC" sub-region. Set the
> > "reg_offset" to 0x1000 for INTC registers.
> >
> > Signed-off-by: Jamin Lin <[email protected]>
> > ---
> > include/hw/intc/aspeed_intc.h | 3 ++-
> > hw/intc/aspeed_intc.c | 50
> ++++++++++++++++++++---------------
> > 2 files changed, 31 insertions(+), 22 deletions(-)
> >
> > diff --git a/include/hw/intc/aspeed_intc.h
> > b/include/hw/intc/aspeed_intc.h index ecaeb15aea..18ca405336 100644
> > --- a/include/hw/intc/aspeed_intc.h
> > +++ b/include/hw/intc/aspeed_intc.h
> > @@ -16,7 +16,7 @@
> > #define TYPE_ASPEED_2700_INTC TYPE_ASPEED_INTC "-ast2700"
> > OBJECT_DECLARE_TYPE(AspeedINTCState, AspeedINTCClass,
> ASPEED_INTC)
> >
> > -#define ASPEED_INTC_NR_REGS (0x2000 >> 2)
> > +#define ASPEED_INTC_NR_REGS (0x808 >> 2)
>
> We should allocate the regs[] array using aic->reg_size to avoid redundancy.
>
Thanks for suggestion.
Will fix it.
Jamin
>
> Thanks,
>
> C.
>
>
>
>
> > #define ASPEED_INTC_NR_INTS 9
> >
> > struct AspeedINTCState {
> > @@ -43,6 +43,7 @@ struct AspeedINTCClass {
> > uint32_t num_ints;
> > uint64_t mem_size;
> > uint64_t reg_size;
> > + uint64_t reg_offset;
> > };
> >
> > #endif /* ASPEED_INTC_H */
> > diff --git a/hw/intc/aspeed_intc.c b/hw/intc/aspeed_intc.c index
> > 316885a27a..0f630ffab7 100644
> > --- a/hw/intc/aspeed_intc.c
> > +++ b/hw/intc/aspeed_intc.c
> > @@ -14,25 +14,31 @@
> > #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)
> > +/*
> > + * INTC Registers
> > + *
> > + * values below are offset by - 0x1000 from datasheet
> > + * because its memory region is start at 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)
> >
> > #define GICINT_STATUS_BASE R_GICINT128_STATUS
> >
> > @@ -311,7 +317,8 @@ static void aspeed_intc_realize(DeviceState *dev,
> Error **errp)
> > memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_intc_ops,
> s,
> > TYPE_ASPEED_INTC ".regs",
> aic->reg_size);
> >
> > - memory_region_add_subregion(&s->iomem_container, 0x0,
> &s->iomem);
> > + memory_region_add_subregion(&s->iomem_container,
> aic->reg_offset,
> > + &s->iomem);
> >
> > qdev_init_gpio_in(dev, aspeed_intc_set_irq, aic->num_ints);
> >
> > @@ -352,7 +359,8 @@ static void aspeed_2700_intc_class_init(ObjectClass
> *klass, void *data)
> > aic->num_lines = 32;
> > aic->num_ints = 9;
> > aic->mem_size = 0x4000;
> > - aic->reg_size = 0x2000;
> > + aic->reg_size = 0x808;
> > + aic->reg_offset = 0x1000;
> > }
> >
> > static const TypeInfo aspeed_2700_intc_info = {
