Re: [PATCH v3 01/28] hw/intc/aspeed: Support setting different memory and register size

[Cédric Le Goater]

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