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 */
> >
