[v3] Add LoongArch softmmu support.

[RFC PATCH v3 18/27] hw/intc: Add LoongArch ls7a interrupt controller support(PCH-PIC)

Posted by Xiaojuan Yang 4 years ago

This patch realize the PCH-PIC interrupt controller.

Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
Signed-off-by: Song Gao <gaosong@loongson.cn>
---
 hw/intc/Kconfig                     |   4 +
 hw/intc/loongarch_pch_pic.c         | 357 ++++++++++++++++++++++++++++
 hw/intc/meson.build                 |   1 +
 hw/intc/trace-events                |   5 +
 hw/loongarch/Kconfig                |   1 +
 include/hw/intc/loongarch_pch_pic.h |  61 +++++
 6 files changed, 429 insertions(+)
 create mode 100644 hw/intc/loongarch_pch_pic.c
 create mode 100644 include/hw/intc/loongarch_pch_pic.h

diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
index 511dcac537..96da13ad1d 100644
--- a/hw/intc/Kconfig
+++ b/hw/intc/Kconfig
@@ -76,3 +76,7 @@ config M68K_IRQC
 
 config LOONGARCH_IPI
     bool
+
+config LOONGARCH_PCH_PIC
+    bool
+    select UNIMP
diff --git a/hw/intc/loongarch_pch_pic.c b/hw/intc/loongarch_pch_pic.c
new file mode 100644
index 0000000000..2ede29ceb0
--- /dev/null
+++ b/hw/intc/loongarch_pch_pic.c
@@ -0,0 +1,357 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * QEMU Loongson 7A1000 I/O interrupt controller.
+ *
+ * Copyright (C) 2021 Loongson Technology Corporation Limited
+ */
+
+#include "qemu/osdep.h"
+#include "hw/sysbus.h"
+#include "hw/loongarch/loongarch.h"
+#include "hw/irq.h"
+#include "hw/intc/loongarch_pch_pic.h"
+#include "migration/vmstate.h"
+#include "trace.h"
+
+#define for_each_set_bit(bit, addr, size) \
+         for ((bit) = find_first_bit((addr), (size));            \
+              (bit) < (size);                                    \
+              (bit) = find_next_bit((addr), (size), (bit) + 1))
+
+static void pch_pic_update_irq(loongarch_pch_pic *s, uint64_t mask, int level)
+{
+    int i;
+    uint64_t val;
+    val = mask & s->intirr & (~s->int_mask);
+
+    for_each_set_bit(i, &val, 64) {
+        if (level == 1) {
+            if ((s->intisr & (0x1ULL << i)) == 0) {
+                s->intisr |= 1ULL << i;
+                qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], 1);
+            }
+        } else if (level == 0) {
+            if (s->intisr & (0x1ULL << i)) {
+                s->intisr &= ~(0x1ULL << i);
+                qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], 0);
+            }
+        }
+    }
+}
+
+static void pch_pic_irq_handler(void *opaque, int irq, int level)
+{
+    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
+
+    assert(irq < PCH_PIC_IRQ_NUM);
+    uint64_t mask = 1ULL << irq;
+
+    trace_pch_pic_irq_handler(s->intedge, irq, level);
+
+    if (s->intedge & mask) {
+        /* Edge triggered */
+        if (level) {
+            if ((s->last_intirr & mask) == 0) {
+                s->intirr |= mask;
+            }
+            s->last_intirr |= mask;
+        } else {
+            s->last_intirr &= ~mask;
+        }
+    } else {
+        /* Level triggered */
+        if (level) {
+            s->intirr |= mask;
+            s->last_intirr |= mask;
+        } else {
+            s->intirr &= ~mask;
+            s->last_intirr &= ~mask;
+        }
+
+    }
+    pch_pic_update_irq(s, mask, level);
+}
+
+static uint64_t loongarch_pch_pic_reg_read(void *opaque, hwaddr addr,
+                                           unsigned size)
+{
+    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
+    uint64_t val = 0;
+    uint32_t offset = addr & 0xfff;
+    int64_t offset_tmp;
+
+    if (size == 8) {
+        switch (offset) {
+        case PCH_PIC_INT_ID_OFFSET:
+            val = (PCH_PIC_INT_ID_NUM << 32) | PCH_PIC_INT_ID_VAL;
+            break;
+        case PCH_PIC_INT_MASK_OFFSET:
+            val =  s->int_mask;
+            break;
+        case PCH_PIC_INT_STATUS_OFFSET:
+            val = s->intisr & (~s->int_mask);
+            break;
+        case PCH_PIC_INT_EDGE_OFFSET:
+            val = s->intedge;
+            break;
+        case PCH_PIC_INT_POL_OFFSET:
+            val = s->int_polarity;
+            break;
+        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
+            val = s->htmsi_en;
+            break;
+        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
+        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
+            break;
+        default:
+            break;
+        }
+    } else if (size == 4) {
+        switch (offset) {
+        case PCH_PIC_INT_ID_OFFSET:
+            val = PCH_PIC_INT_ID_VAL;
+            break;
+        case PCH_PIC_INT_ID_OFFSET + 4:
+            val = PCH_PIC_INT_ID_NUM;
+            break;
+        case PCH_PIC_INT_MASK_OFFSET...PCH_PIC_INT_MASK_END:
+            val = ldl_p((void *)&s->int_mask +
+                        (offset - PCH_PIC_INT_MASK_OFFSET));
+            break;
+        case PCH_PIC_INT_STATUS_OFFSET...PCH_PIC_INT_STATUS_END:
+            val = ldl_p((void *)&s->intisr +
+                        (offset - PCH_PIC_INT_STATUS_OFFSET)) & (~s->int_mask);
+            break;
+        case PCH_PIC_INT_EDGE_OFFSET...PCH_PIC_INT_EDGE_END:
+            val = ldl_p((void *)&s->intedge +
+                        (offset - PCH_PIC_INT_EDGE_OFFSET));
+            break;
+        case PCH_PIC_INT_POL_OFFSET...PCH_PIC_INT_POL_END:
+            val = ldl_p((void *)&s->int_polarity +
+                        (offset - PCH_PIC_INT_POL_OFFSET));
+            break;
+        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
+            val = ldl_p((void *)&s->htmsi_en +
+                        (offset - PCH_PIC_HTMSI_EN_OFFSET));
+            break;
+        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
+        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
+            break;
+        default:
+            break;
+        }
+    } else if (size == 1) {
+        if (offset >= PCH_PIC_HTMSI_VEC_OFFSET) {
+            offset_tmp = offset - PCH_PIC_HTMSI_VEC_OFFSET;
+            if (offset_tmp >= 0 && offset_tmp < 64) {
+                val = s->htmsi_vector[offset_tmp];
+            }
+        } else if (offset >=  PCH_PIC_ROUTE_ENTRY_OFFSET) {
+            offset_tmp = offset - PCH_PIC_ROUTE_ENTRY_OFFSET;
+            if (offset_tmp >= 0 && offset_tmp < 64) {
+                val = s->route_entry[offset_tmp];
+            }
+        }
+    }
+
+    trace_loongarch_pch_pic_read(size, (uint32_t)addr, val);
+    return val;
+}
+
+static void loongarch_pch_pic_reg_write(void *opaque, hwaddr addr,
+                                        uint64_t data, unsigned size)
+{
+    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
+    int32_t offset_tmp;
+    uint32_t offset, old;
+    offset = addr & 0xfff;
+
+    trace_loongarch_pch_pic_write(size, (uint32_t)addr, data);
+
+    if (size == 8) {
+        switch (offset) {
+        case PCH_PIC_INT_MASK_OFFSET:
+            old = s->int_mask;
+            s->int_mask = data;
+            if (old & ~data) {
+                pch_pic_update_irq(s, (old & ~data), 1);
+            } else if (~old & data) {
+                pch_pic_update_irq(s, (~old & data), 0);
+            }
+            break;
+        case PCH_PIC_INT_STATUS_OFFSET:
+            s->intisr = data;
+            break;
+        case PCH_PIC_INT_EDGE_OFFSET:
+            s->intedge = data;
+            break;
+        case PCH_PIC_INT_CLEAR_OFFSET:
+            s->intirr &= (~(data & s->intedge));
+            pch_pic_update_irq(s, data, 0);
+            s->intisr &= (~data);
+            break;
+        case PCH_PIC_INT_POL_OFFSET:
+            s->int_polarity = data;
+            break;
+        case PCH_PIC_HTMSI_EN_OFFSET:
+            s->htmsi_en = data;
+            break;
+        case PCH_PIC_AUTO_CTRL0_OFFSET:
+        case PCH_PIC_AUTO_CTRL1_OFFSET:
+            break;
+        default:
+            break;
+        }
+    } else if (size == 4) {
+        switch (offset) {
+        case PCH_PIC_INT_MASK_OFFSET...PCH_PIC_INT_MASK_END:
+            offset -= PCH_PIC_INT_MASK_OFFSET;
+            old = ldl_p((void *)&s->int_mask + offset);
+            stl_p((void *)&s->int_mask + offset, data);
+
+            if (old & ~data) {
+                pch_pic_update_irq(s, (old & ~data), 1);
+            } else if (~old & data) {
+                pch_pic_update_irq(s, (~old & data), 0);
+            }
+            break;
+        case PCH_PIC_INT_STATUS_OFFSET...PCH_PIC_INT_STATUS_END:
+            stl_p((void *)&s->intisr + (offset - PCH_PIC_INT_STATUS_OFFSET),
+                  data);
+            break;
+        case PCH_PIC_INT_EDGE_OFFSET...PCH_PIC_INT_EDGE_END:
+            stl_p((void *)&s->intedge + (offset - PCH_PIC_INT_EDGE_OFFSET),
+                  data);
+            break;
+        case PCH_PIC_INT_CLEAR_OFFSET...PCH_PIC_INT_CLEAR_END:
+            old = s->intirr & (~(data & s->intedge));
+            stl_p((void *)&s->intirr + (offset - PCH_PIC_INT_CLEAR_OFFSET),
+                  old);
+            pch_pic_update_irq(s, data, 0);
+            old = s->intisr & (~data);
+            stl_p((void *)&s->intisr + (offset - PCH_PIC_INT_CLEAR_OFFSET),
+                  old);
+            break;
+        case PCH_PIC_INT_POL_OFFSET...PCH_PIC_INT_POL_END:
+            stl_p((void *)&s->int_polarity + (offset - PCH_PIC_INT_POL_OFFSET),
+                  data);
+            break;
+        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
+            stl_p((void *)&s->htmsi_en + (offset - PCH_PIC_HTMSI_EN_OFFSET),
+                  data);
+            break;
+        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
+        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
+            break;
+        default:
+            break;
+        }
+    } else if (size == 1) {
+        if (offset >= PCH_PIC_HTMSI_VEC_OFFSET) {
+            offset_tmp = offset - PCH_PIC_HTMSI_VEC_OFFSET;
+            if (offset_tmp >= 0 && offset_tmp < 64) {
+                s->htmsi_vector[offset_tmp] = (uint8_t)(data & 0xff);
+            }
+        } else if (offset >=  PCH_PIC_ROUTE_ENTRY_OFFSET) {
+            offset_tmp = offset - PCH_PIC_ROUTE_ENTRY_OFFSET;
+            if (offset_tmp >= 0 && offset_tmp < 64) {
+                s->route_entry[offset_tmp] = (uint8_t)(data & 0xff);
+            }
+        }
+    }
+}
+
+static const MemoryRegionOps loongarch_pch_pic_ops = {
+    .read = loongarch_pch_pic_reg_read,
+    .write = loongarch_pch_pic_reg_write,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 8,
+    },
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 8,
+    },
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void loongarch_pch_pic_reset(DeviceState *d)
+{
+    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(d);
+    int i;
+
+    s->int_mask = -1ULL;
+    s->htmsi_en = 0x0;
+    s->intedge  = 0x0;
+    s->intclr   = 0x0;
+    s->auto_crtl0 = 0x0;
+    s->auto_crtl1 = 0x0;
+    for (i = 0; i < 64; i++) {
+        s->route_entry[i] = 0x1;
+        s->htmsi_vector[i] = 0x0;
+    }
+    s->intirr = 0x0;
+    s->intisr = 0x0;
+    s->last_intirr = 0x0;
+    s->int_polarity = 0x0;
+}
+
+static void loongarch_pch_pic_init(Object *obj)
+{
+    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    int i;
+
+    memory_region_init_io(&s->iomem, obj, &loongarch_pch_pic_ops,
+                          s, TYPE_LOONGARCH_PCH_PIC, 0x1000);
+    sysbus_init_mmio(sbd, &s->iomem);
+
+    for (i = 0; i < PCH_PIC_IRQ_NUM; i++) {
+        sysbus_init_irq(sbd, &s->parent_irq[i]);
+    }
+    qdev_init_gpio_in(DEVICE(obj), pch_pic_irq_handler, PCH_PIC_IRQ_NUM);
+}
+
+static const VMStateDescription vmstate_loongarch_pch_pic = {
+    .name = TYPE_LOONGARCH_PCH_PIC,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(int_mask, loongarch_pch_pic),
+        VMSTATE_UINT64(htmsi_en, loongarch_pch_pic),
+        VMSTATE_UINT64(intedge, loongarch_pch_pic),
+        VMSTATE_UINT64(intclr, loongarch_pch_pic),
+        VMSTATE_UINT64(auto_crtl0, loongarch_pch_pic),
+        VMSTATE_UINT64(auto_crtl1, loongarch_pch_pic),
+        VMSTATE_UINT8_ARRAY(route_entry, loongarch_pch_pic, 64),
+        VMSTATE_UINT8_ARRAY(htmsi_vector, loongarch_pch_pic, 64),
+        VMSTATE_UINT64(last_intirr, loongarch_pch_pic),
+        VMSTATE_UINT64(intirr, loongarch_pch_pic),
+        VMSTATE_UINT64(intisr, loongarch_pch_pic),
+        VMSTATE_UINT64(int_polarity, loongarch_pch_pic),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void loongarch_pch_pic_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = loongarch_pch_pic_reset;
+    dc->vmsd = &vmstate_loongarch_pch_pic;
+}
+
+static const TypeInfo loongarch_pch_pic_info = {
+    .name          = TYPE_LOONGARCH_PCH_PIC,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(loongarch_pch_pic),
+    .instance_init = loongarch_pch_pic_init,
+    .class_init    = loongarch_pch_pic_class_init,
+};
+
+static void loongarch_pch_pic_register_types(void)
+{
+    type_register_static(&loongarch_pch_pic_info);
+}
+
+type_init(loongarch_pch_pic_register_types)
diff --git a/hw/intc/meson.build b/hw/intc/meson.build
index 51f0c3988a..33ba63266e 100644
--- a/hw/intc/meson.build
+++ b/hw/intc/meson.build
@@ -58,3 +58,4 @@ specific_ss.add(when: ['CONFIG_KVM', 'CONFIG_XIVE'],
 specific_ss.add(when: 'CONFIG_GOLDFISH_PIC', if_true: files('goldfish_pic.c'))
 specific_ss.add(when: 'CONFIG_M68K_IRQC', if_true: files('m68k_irqc.c'))
 specific_ss.add(when: 'CONFIG_LOONGARCH_IPI', if_true: files('loongarch_ipi.c'))
+specific_ss.add(when: 'CONFIG_LOONGARCH_PCH_PIC', if_true: files('loongarch_pch_pic.c'))
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 124608e51f..52fedf82be 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -250,3 +250,8 @@ sh_intc_set(int id, int enable) "setting interrupt group %d to %d"
 # loongarch_ipi.c
 loongarch_ipi_read(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%"PRIx64
 loongarch_ipi_write(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%"PRIx64
+
+# loongarch_pch_pic.c
+pch_pic_irq_handler(uint32_t edge, int irq, int level) "edge 0x%02x irq %d level %d"
+loongarch_pch_pic_read(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx64
+loongarch_pch_pic_write(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx64
diff --git a/hw/loongarch/Kconfig b/hw/loongarch/Kconfig
index 1591574397..c2b8046b94 100644
--- a/hw/loongarch/Kconfig
+++ b/hw/loongarch/Kconfig
@@ -2,3 +2,4 @@ config LOONGSON3_LS7A
     bool
     select PCI_EXPRESS_7A
     select LOONGARCH_IPI
+    select LOONGARCH_PCH_PIC
diff --git a/include/hw/intc/loongarch_pch_pic.h b/include/hw/intc/loongarch_pch_pic.h
new file mode 100644
index 0000000000..bc04ed28ef
--- /dev/null
+++ b/include/hw/intc/loongarch_pch_pic.h
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * LoongArch 7A1000 I/O interrupt controller definitions
+ *
+ * Copyright (c) 2021 Loongson Technology Corporation Limited
+ */
+
+#define TYPE_LOONGARCH_PCH_PIC "loongarch_pch_pic"
+DECLARE_INSTANCE_CHECKER(struct loongarch_pch_pic, LOONGARCH_PCH_PIC,
+                         TYPE_LOONGARCH_PCH_PIC)
+
+#define PCH_PIC_IRQ_START               0
+#define PCH_PIC_IRQ_END                 63
+#define PCH_PIC_IRQ_NUM                 64
+#define PCH_PIC_INT_ID_VAL              0x7000000UL
+#define PCH_PIC_INT_ID_NUM              0x3f0001UL
+
+#define PCH_PIC_INT_ID_OFFSET           0x00
+#define PCH_PIC_INT_ID_END              0x07
+#define PCH_PIC_INT_MASK_OFFSET         0x20
+#define PCH_PIC_INT_MASK_END            0x27
+#define PCH_PIC_HTMSI_EN_OFFSET         0x40
+#define PCH_PIC_HTMSI_EN_END            0x47
+#define PCH_PIC_INT_EDGE_OFFSET         0x60
+#define PCH_PIC_INT_EDGE_END            0x67
+#define PCH_PIC_INT_CLEAR_OFFSET        0x80
+#define PCH_PIC_INT_CLEAR_END           0x87
+#define PCH_PIC_AUTO_CTRL0_OFFSET       0xc0
+#define PCH_PIC_AUTO_CTRL0_END          0xc7
+#define PCH_PIC_AUTO_CTRL1_OFFSET       0xe0
+#define PCH_PIC_AUTO_CTRL1_END          0xe8
+#define PCH_PIC_ROUTE_ENTRY_OFFSET      0x100
+#define PCH_PIC_ROUTE_ENTRY_END         0x13f
+#define PCH_PIC_HTMSI_VEC_OFFSET        0x200
+#define PCH_PIC_HTMSI_VEC_END           0x23f
+#define PCH_PIC_INT_STATUS_OFFSET       0x3a0
+#define PCH_PIC_INT_STATUS_END          0x3a7
+#define PCH_PIC_INT_POL_OFFSET          0x3e0
+#define PCH_PIC_INT_POL_END             0x3e7
+
+typedef struct loongarch_pch_pic {
+    SysBusDevice parent_obj;
+    qemu_irq parent_irq[64];
+    uint64_t int_mask; /*0x020 interrupt mask register*/
+    uint64_t htmsi_en;/*0x040 1=msi*/
+    uint64_t intedge; /*0x060 edge=1 level  =0*/
+    uint64_t intclr; /*0x080 for clean edge int,set 1 clean,set 0 is noused*/
+    uint64_t auto_crtl0; /*0x0c0*/
+    uint64_t auto_crtl1; /*0x0e0*/
+    uint8_t route_entry[64]; /*0x100 - 0x138*/
+    uint8_t htmsi_vector[64]; /*0x200 - 0x238*/
+    uint64_t last_intirr;    /* edge detection */
+    uint64_t intirr; /* 0x380 interrupt request register */
+    uint64_t intisr; /* 0x3a0 interrupt service register */
+    /*
+     * 0x3e0 interrupt level polarity selection
+     * register 0 for high level trigger
+     */
+    uint64_t int_polarity;
+    MemoryRegion iomem;
+} loongarch_pch_pic;
-- 
2.27.0

Re: [RFC PATCH v3 18/27] hw/intc: Add LoongArch ls7a interrupt controller support(PCH-PIC)

Posted by Mark Cave-Ayland 4 years ago

On 04/12/2021 12:07, Xiaojuan Yang wrote:

> This patch realize the PCH-PIC interrupt controller.
> 
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> ---
>   hw/intc/Kconfig                     |   4 +
>   hw/intc/loongarch_pch_pic.c         | 357 ++++++++++++++++++++++++++++
>   hw/intc/meson.build                 |   1 +
>   hw/intc/trace-events                |   5 +
>   hw/loongarch/Kconfig                |   1 +
>   include/hw/intc/loongarch_pch_pic.h |  61 +++++
>   6 files changed, 429 insertions(+)
>   create mode 100644 hw/intc/loongarch_pch_pic.c
>   create mode 100644 include/hw/intc/loongarch_pch_pic.h
> 
> diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
> index 511dcac537..96da13ad1d 100644
> --- a/hw/intc/Kconfig
> +++ b/hw/intc/Kconfig
> @@ -76,3 +76,7 @@ config M68K_IRQC
>   
>   config LOONGARCH_IPI
>       bool
> +
> +config LOONGARCH_PCH_PIC
> +    bool
> +    select UNIMP
> diff --git a/hw/intc/loongarch_pch_pic.c b/hw/intc/loongarch_pch_pic.c
> new file mode 100644
> index 0000000000..2ede29ceb0
> --- /dev/null
> +++ b/hw/intc/loongarch_pch_pic.c
> @@ -0,0 +1,357 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * QEMU Loongson 7A1000 I/O interrupt controller.
> + *
> + * Copyright (C) 2021 Loongson Technology Corporation Limited
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/sysbus.h"
> +#include "hw/loongarch/loongarch.h"
> +#include "hw/irq.h"
> +#include "hw/intc/loongarch_pch_pic.h"
> +#include "migration/vmstate.h"
> +#include "trace.h"
> +
> +#define for_each_set_bit(bit, addr, size) \
> +         for ((bit) = find_first_bit((addr), (size));            \
> +              (bit) < (size);                                    \
> +              (bit) = find_next_bit((addr), (size), (bit) + 1))
> +
> +static void pch_pic_update_irq(loongarch_pch_pic *s, uint64_t mask, int level)
> +{
> +    int i;
> +    uint64_t val;
> +    val = mask & s->intirr & (~s->int_mask);
> +
> +    for_each_set_bit(i, &val, 64) {
> +        if (level == 1) {
> +            if ((s->intisr & (0x1ULL << i)) == 0) {
> +                s->intisr |= 1ULL << i;
> +                qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], 1);
> +            }
> +        } else if (level == 0) {
> +            if (s->intisr & (0x1ULL << i)) {
> +                s->intisr &= ~(0x1ULL << i);
> +                qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], 0);
> +            }
> +        }
> +    }
> +}

The normal pattern would be to use something like:

for (i = 0; i < 64; i++) {
     if (level) {
         s->intisr |= 1ULL << i;
     } else {
         s->intisr &= ~(0x1ULL << i);
     }

     qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], level);
}

Why is it necessary to check the previous value of (s->intisr & (0x1ULL << i)) here?

> +static void pch_pic_irq_handler(void *opaque, int irq, int level)
> +{
> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
> +
> +    assert(irq < PCH_PIC_IRQ_NUM);
> +    uint64_t mask = 1ULL << irq;
> +
> +    trace_pch_pic_irq_handler(s->intedge, irq, level);
> +
> +    if (s->intedge & mask) {
> +        /* Edge triggered */
> +        if (level) {
> +            if ((s->last_intirr & mask) == 0) {
> +                s->intirr |= mask;
> +            }
> +            s->last_intirr |= mask;
> +        } else {
> +            s->last_intirr &= ~mask;
> +        }
> +    } else {
> +        /* Level triggered */
> +        if (level) {
> +            s->intirr |= mask;
> +            s->last_intirr |= mask;
> +        } else {
> +            s->intirr &= ~mask;
> +            s->last_intirr &= ~mask;
> +        }
> +
> +    }
> +    pch_pic_update_irq(s, mask, level);
> +}
> +
> +static uint64_t loongarch_pch_pic_reg_read(void *opaque, hwaddr addr,
> +                                           unsigned size)
> +{
> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
> +    uint64_t val = 0;
> +    uint32_t offset = addr & 0xfff;
> +    int64_t offset_tmp;
> +
> +    if (size == 8) {
> +        switch (offset) {
> +        case PCH_PIC_INT_ID_OFFSET:
> +            val = (PCH_PIC_INT_ID_NUM << 32) | PCH_PIC_INT_ID_VAL;
> +            break;
> +        case PCH_PIC_INT_MASK_OFFSET:
> +            val =  s->int_mask;
> +            break;
> +        case PCH_PIC_INT_STATUS_OFFSET:
> +            val = s->intisr & (~s->int_mask);
> +            break;
> +        case PCH_PIC_INT_EDGE_OFFSET:
> +            val = s->intedge;
> +            break;
> +        case PCH_PIC_INT_POL_OFFSET:
> +            val = s->int_polarity;
> +            break;
> +        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
> +            val = s->htmsi_en;
> +            break;
> +        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
> +        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
> +            break;
> +        default:
> +            break;
> +        }
> +    } else if (size == 4) {
> +        switch (offset) {
> +        case PCH_PIC_INT_ID_OFFSET:
> +            val = PCH_PIC_INT_ID_VAL;
> +            break;
> +        case PCH_PIC_INT_ID_OFFSET + 4:
> +            val = PCH_PIC_INT_ID_NUM;
> +            break;
> +        case PCH_PIC_INT_MASK_OFFSET...PCH_PIC_INT_MASK_END:
> +            val = ldl_p((void *)&s->int_mask +
> +                        (offset - PCH_PIC_INT_MASK_OFFSET));
> +            break;
> +        case PCH_PIC_INT_STATUS_OFFSET...PCH_PIC_INT_STATUS_END:
> +            val = ldl_p((void *)&s->intisr +
> +                        (offset - PCH_PIC_INT_STATUS_OFFSET)) & (~s->int_mask);
> +            break;
> +        case PCH_PIC_INT_EDGE_OFFSET...PCH_PIC_INT_EDGE_END:
> +            val = ldl_p((void *)&s->intedge +
> +                        (offset - PCH_PIC_INT_EDGE_OFFSET));
> +            break;
> +        case PCH_PIC_INT_POL_OFFSET...PCH_PIC_INT_POL_END:
> +            val = ldl_p((void *)&s->int_polarity +
> +                        (offset - PCH_PIC_INT_POL_OFFSET));
> +            break;
> +        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
> +            val = ldl_p((void *)&s->htmsi_en +
> +                        (offset - PCH_PIC_HTMSI_EN_OFFSET));
> +            break;
> +        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
> +        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
> +            break;
> +        default:
> +            break;
> +        }
> +    } else if (size == 1) {
> +        if (offset >= PCH_PIC_HTMSI_VEC_OFFSET) {
> +            offset_tmp = offset - PCH_PIC_HTMSI_VEC_OFFSET;
> +            if (offset_tmp >= 0 && offset_tmp < 64) {
> +                val = s->htmsi_vector[offset_tmp];
> +            }
> +        } else if (offset >=  PCH_PIC_ROUTE_ENTRY_OFFSET) {
> +            offset_tmp = offset - PCH_PIC_ROUTE_ENTRY_OFFSET;
> +            if (offset_tmp >= 0 && offset_tmp < 64) {
> +                val = s->route_entry[offset_tmp];
> +            }
> +        }
> +    }
> +
> +    trace_loongarch_pch_pic_read(size, (uint32_t)addr, val);
> +    return val;
> +}
> +
> +static void loongarch_pch_pic_reg_write(void *opaque, hwaddr addr,
> +                                        uint64_t data, unsigned size)
> +{
> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);

Perhaps use LoongArchPCHPIC for the camel case version?

> +    int32_t offset_tmp;
> +    uint32_t offset, old;
> +    offset = addr & 0xfff;
> +
> +    trace_loongarch_pch_pic_write(size, (uint32_t)addr, data);
> +
> +    if (size == 8) {
> +        switch (offset) {
> +        case PCH_PIC_INT_MASK_OFFSET:
> +            old = s->int_mask;
> +            s->int_mask = data;
> +            if (old & ~data) {
> +                pch_pic_update_irq(s, (old & ~data), 1);
> +            } else if (~old & data) {
> +                pch_pic_update_irq(s, (~old & data), 0);
> +            }
> +            break;
> +        case PCH_PIC_INT_STATUS_OFFSET:
> +            s->intisr = data;
> +            break;
> +        case PCH_PIC_INT_EDGE_OFFSET:
> +            s->intedge = data;
> +            break;
> +        case PCH_PIC_INT_CLEAR_OFFSET:
> +            s->intirr &= (~(data & s->intedge));
> +            pch_pic_update_irq(s, data, 0);
> +            s->intisr &= (~data);
> +            break;
> +        case PCH_PIC_INT_POL_OFFSET:
> +            s->int_polarity = data;
> +            break;
> +        case PCH_PIC_HTMSI_EN_OFFSET:
> +            s->htmsi_en = data;
> +            break;
> +        case PCH_PIC_AUTO_CTRL0_OFFSET:
> +        case PCH_PIC_AUTO_CTRL1_OFFSET:
> +            break;
> +        default:
> +            break;
> +        }
> +    } else if (size == 4) {
> +        switch (offset) {
> +        case PCH_PIC_INT_MASK_OFFSET...PCH_PIC_INT_MASK_END:
> +            offset -= PCH_PIC_INT_MASK_OFFSET;
> +            old = ldl_p((void *)&s->int_mask + offset);
> +            stl_p((void *)&s->int_mask + offset, data);
> +
> +            if (old & ~data) {
> +                pch_pic_update_irq(s, (old & ~data), 1);
> +            } else if (~old & data) {
> +                pch_pic_update_irq(s, (~old & data), 0);
> +            }
> +            break;
> +        case PCH_PIC_INT_STATUS_OFFSET...PCH_PIC_INT_STATUS_END:
> +            stl_p((void *)&s->intisr + (offset - PCH_PIC_INT_STATUS_OFFSET),
> +                  data);
> +            break;
> +        case PCH_PIC_INT_EDGE_OFFSET...PCH_PIC_INT_EDGE_END:
> +            stl_p((void *)&s->intedge + (offset - PCH_PIC_INT_EDGE_OFFSET),
> +                  data);
> +            break;
> +        case PCH_PIC_INT_CLEAR_OFFSET...PCH_PIC_INT_CLEAR_END:
> +            old = s->intirr & (~(data & s->intedge));
> +            stl_p((void *)&s->intirr + (offset - PCH_PIC_INT_CLEAR_OFFSET),
> +                  old);
> +            pch_pic_update_irq(s, data, 0);
> +            old = s->intisr & (~data);
> +            stl_p((void *)&s->intisr + (offset - PCH_PIC_INT_CLEAR_OFFSET),
> +                  old);
> +            break;
> +        case PCH_PIC_INT_POL_OFFSET...PCH_PIC_INT_POL_END:
> +            stl_p((void *)&s->int_polarity + (offset - PCH_PIC_INT_POL_OFFSET),
> +                  data);
> +            break;
> +        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
> +            stl_p((void *)&s->htmsi_en + (offset - PCH_PIC_HTMSI_EN_OFFSET),
> +                  data);
> +            break;
> +        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
> +        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
> +            break;
> +        default:
> +            break;
> +        }
> +    } else if (size == 1) {
> +        if (offset >= PCH_PIC_HTMSI_VEC_OFFSET) {
> +            offset_tmp = offset - PCH_PIC_HTMSI_VEC_OFFSET;
> +            if (offset_tmp >= 0 && offset_tmp < 64) {
> +                s->htmsi_vector[offset_tmp] = (uint8_t)(data & 0xff);
> +            }
> +        } else if (offset >=  PCH_PIC_ROUTE_ENTRY_OFFSET) {
> +            offset_tmp = offset - PCH_PIC_ROUTE_ENTRY_OFFSET;
> +            if (offset_tmp >= 0 && offset_tmp < 64) {
> +                s->route_entry[offset_tmp] = (uint8_t)(data & 0xff);
> +            }
> +        }
> +    }
> +}
> +
> +static const MemoryRegionOps loongarch_pch_pic_ops = {
> +    .read = loongarch_pch_pic_reg_read,
> +    .write = loongarch_pch_pic_reg_write,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 8,
> +    },
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 8,
> +    },
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};

It feels like this should be split into 2 separate memory regions: as per before set 
.min_access_size = 4 for PCH_PIC_INT_MASK_OFFSET ... PCH_PIC_AUTO_CTRL1_END and use 
uint32_t for the underlying types in loongarch_pch_pic.

This leaves a separate memory region for accesses > PCH_PIC_HTMSI_VEC_OFFSET which 
can be set with .min_access_size = 1 which maps nicely to the underlying uint8_t 
types. Which means you should be able to access the arrays directly instead of having 
to use ldl_p() and stl_p().

> +static void loongarch_pch_pic_reset(DeviceState *d)
> +{
> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(d);
> +    int i;
> +
> +    s->int_mask = -1ULL;
> +    s->htmsi_en = 0x0;
> +    s->intedge  = 0x0;
> +    s->intclr   = 0x0;
> +    s->auto_crtl0 = 0x0;
> +    s->auto_crtl1 = 0x0;
> +    for (i = 0; i < 64; i++) {
> +        s->route_entry[i] = 0x1;
> +        s->htmsi_vector[i] = 0x0;
> +    }
> +    s->intirr = 0x0;
> +    s->intisr = 0x0;
> +    s->last_intirr = 0x0;
> +    s->int_polarity = 0x0;
> +}
> +
> +static void loongarch_pch_pic_init(Object *obj)
> +{
> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    int i;
> +
> +    memory_region_init_io(&s->iomem, obj, &loongarch_pch_pic_ops,
> +                          s, TYPE_LOONGARCH_PCH_PIC, 0x1000);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +
> +    for (i = 0; i < PCH_PIC_IRQ_NUM; i++) {
> +        sysbus_init_irq(sbd, &s->parent_irq[i]);
> +    }
> +    qdev_init_gpio_in(DEVICE(obj), pch_pic_irq_handler, PCH_PIC_IRQ_NUM);
> +}
> +
> +static const VMStateDescription vmstate_loongarch_pch_pic = {
> +    .name = TYPE_LOONGARCH_PCH_PIC,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(int_mask, loongarch_pch_pic),
> +        VMSTATE_UINT64(htmsi_en, loongarch_pch_pic),
> +        VMSTATE_UINT64(intedge, loongarch_pch_pic),
> +        VMSTATE_UINT64(intclr, loongarch_pch_pic),
> +        VMSTATE_UINT64(auto_crtl0, loongarch_pch_pic),
> +        VMSTATE_UINT64(auto_crtl1, loongarch_pch_pic),
> +        VMSTATE_UINT8_ARRAY(route_entry, loongarch_pch_pic, 64),
> +        VMSTATE_UINT8_ARRAY(htmsi_vector, loongarch_pch_pic, 64),
> +        VMSTATE_UINT64(last_intirr, loongarch_pch_pic),
> +        VMSTATE_UINT64(intirr, loongarch_pch_pic),
> +        VMSTATE_UINT64(intisr, loongarch_pch_pic),
> +        VMSTATE_UINT64(int_polarity, loongarch_pch_pic),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void loongarch_pch_pic_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = loongarch_pch_pic_reset;
> +    dc->vmsd = &vmstate_loongarch_pch_pic;
> +}
> +
> +static const TypeInfo loongarch_pch_pic_info = {
> +    .name          = TYPE_LOONGARCH_PCH_PIC,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(loongarch_pch_pic),
> +    .instance_init = loongarch_pch_pic_init,
> +    .class_init    = loongarch_pch_pic_class_init,
> +};
> +
> +static void loongarch_pch_pic_register_types(void)
> +{
> +    type_register_static(&loongarch_pch_pic_info);
> +}
> +
> +type_init(loongarch_pch_pic_register_types)
> diff --git a/hw/intc/meson.build b/hw/intc/meson.build
> index 51f0c3988a..33ba63266e 100644
> --- a/hw/intc/meson.build
> +++ b/hw/intc/meson.build
> @@ -58,3 +58,4 @@ specific_ss.add(when: ['CONFIG_KVM', 'CONFIG_XIVE'],
>   specific_ss.add(when: 'CONFIG_GOLDFISH_PIC', if_true: files('goldfish_pic.c'))
>   specific_ss.add(when: 'CONFIG_M68K_IRQC', if_true: files('m68k_irqc.c'))
>   specific_ss.add(when: 'CONFIG_LOONGARCH_IPI', if_true: files('loongarch_ipi.c'))
> +specific_ss.add(when: 'CONFIG_LOONGARCH_PCH_PIC', if_true: files('loongarch_pch_pic.c'))
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index 124608e51f..52fedf82be 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -250,3 +250,8 @@ sh_intc_set(int id, int enable) "setting interrupt group %d to %d"
>   # loongarch_ipi.c
>   loongarch_ipi_read(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%"PRIx64
>   loongarch_ipi_write(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%"PRIx64
> +
> +# loongarch_pch_pic.c
> +pch_pic_irq_handler(uint32_t edge, int irq, int level) "edge 0x%02x irq %d level %d"
> +loongarch_pch_pic_read(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx64
> +loongarch_pch_pic_write(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx64
> diff --git a/hw/loongarch/Kconfig b/hw/loongarch/Kconfig
> index 1591574397..c2b8046b94 100644
> --- a/hw/loongarch/Kconfig
> +++ b/hw/loongarch/Kconfig
> @@ -2,3 +2,4 @@ config LOONGSON3_LS7A
>       bool
>       select PCI_EXPRESS_7A
>       select LOONGARCH_IPI
> +    select LOONGARCH_PCH_PIC
> diff --git a/include/hw/intc/loongarch_pch_pic.h b/include/hw/intc/loongarch_pch_pic.h
> new file mode 100644
> index 0000000000..bc04ed28ef
> --- /dev/null
> +++ b/include/hw/intc/loongarch_pch_pic.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * LoongArch 7A1000 I/O interrupt controller definitions
> + *
> + * Copyright (c) 2021 Loongson Technology Corporation Limited
> + */
> +
> +#define TYPE_LOONGARCH_PCH_PIC "loongarch_pch_pic"
> +DECLARE_INSTANCE_CHECKER(struct loongarch_pch_pic, LOONGARCH_PCH_PIC,
> +                         TYPE_LOONGARCH_PCH_PIC)
> +
> +#define PCH_PIC_IRQ_START               0
> +#define PCH_PIC_IRQ_END                 63
> +#define PCH_PIC_IRQ_NUM                 64
> +#define PCH_PIC_INT_ID_VAL              0x7000000UL
> +#define PCH_PIC_INT_ID_NUM              0x3f0001UL
> +
> +#define PCH_PIC_INT_ID_OFFSET           0x00
> +#define PCH_PIC_INT_ID_END              0x07
> +#define PCH_PIC_INT_MASK_OFFSET         0x20
> +#define PCH_PIC_INT_MASK_END            0x27
> +#define PCH_PIC_HTMSI_EN_OFFSET         0x40
> +#define PCH_PIC_HTMSI_EN_END            0x47
> +#define PCH_PIC_INT_EDGE_OFFSET         0x60
> +#define PCH_PIC_INT_EDGE_END            0x67
> +#define PCH_PIC_INT_CLEAR_OFFSET        0x80
> +#define PCH_PIC_INT_CLEAR_END           0x87
> +#define PCH_PIC_AUTO_CTRL0_OFFSET       0xc0
> +#define PCH_PIC_AUTO_CTRL0_END          0xc7
> +#define PCH_PIC_AUTO_CTRL1_OFFSET       0xe0
> +#define PCH_PIC_AUTO_CTRL1_END          0xe8
> +#define PCH_PIC_ROUTE_ENTRY_OFFSET      0x100
> +#define PCH_PIC_ROUTE_ENTRY_END         0x13f
> +#define PCH_PIC_HTMSI_VEC_OFFSET        0x200
> +#define PCH_PIC_HTMSI_VEC_END           0x23f
> +#define PCH_PIC_INT_STATUS_OFFSET       0x3a0
> +#define PCH_PIC_INT_STATUS_END          0x3a7
> +#define PCH_PIC_INT_POL_OFFSET          0x3e0
> +#define PCH_PIC_INT_POL_END             0x3e7
> +
> +typedef struct loongarch_pch_pic {
> +    SysBusDevice parent_obj;
> +    qemu_irq parent_irq[64];
> +    uint64_t int_mask; /*0x020 interrupt mask register*/
> +    uint64_t htmsi_en;/*0x040 1=msi*/
> +    uint64_t intedge; /*0x060 edge=1 level  =0*/
> +    uint64_t intclr; /*0x080 for clean edge int,set 1 clean,set 0 is noused*/
> +    uint64_t auto_crtl0; /*0x0c0*/
> +    uint64_t auto_crtl1; /*0x0e0*/
> +    uint8_t route_entry[64]; /*0x100 - 0x138*/
> +    uint8_t htmsi_vector[64]; /*0x200 - 0x238*/
> +    uint64_t last_intirr;    /* edge detection */
> +    uint64_t intirr; /* 0x380 interrupt request register */
> +    uint64_t intisr; /* 0x3a0 interrupt service register */
> +    /*
> +     * 0x3e0 interrupt level polarity selection
> +     * register 0 for high level trigger
> +     */
> +    uint64_t int_polarity;
> +    MemoryRegion iomem;
> +} loongarch_pch_pic;


ATB,

Mark.

Re: [RFC PATCH v3 18/27] hw/intc: Add LoongArch ls7a interrupt controller support(PCH-PIC)

Posted by yangxiaojuan 3 years, 12 months ago

Hi, Mark

On 12/18/2021 08:33 AM, Mark Cave-Ayland wrote:
> On 04/12/2021 12:07, Xiaojuan Yang wrote:
> 
>> This patch realize the PCH-PIC interrupt controller.
>>
>> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
>> Signed-off-by: Song Gao <gaosong@loongson.cn>
>> ---
>>   hw/intc/Kconfig                     |   4 +
>>   hw/intc/loongarch_pch_pic.c         | 357 ++++++++++++++++++++++++++++
>>   hw/intc/meson.build                 |   1 +
>>   hw/intc/trace-events                |   5 +
>>   hw/loongarch/Kconfig                |   1 +
>>   include/hw/intc/loongarch_pch_pic.h |  61 +++++
>>   6 files changed, 429 insertions(+)
>>   create mode 100644 hw/intc/loongarch_pch_pic.c
>>   create mode 100644 include/hw/intc/loongarch_pch_pic.h
>>
>> diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
>> index 511dcac537..96da13ad1d 100644
>> --- a/hw/intc/Kconfig
>> +++ b/hw/intc/Kconfig
>> @@ -76,3 +76,7 @@ config M68K_IRQC
>>     config LOONGARCH_IPI
>>       bool
>> +
>> +config LOONGARCH_PCH_PIC
>> +    bool
>> +    select UNIMP
>> diff --git a/hw/intc/loongarch_pch_pic.c b/hw/intc/loongarch_pch_pic.c
>> new file mode 100644
>> index 0000000000..2ede29ceb0
>> --- /dev/null
>> +++ b/hw/intc/loongarch_pch_pic.c
>> @@ -0,0 +1,357 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * QEMU Loongson 7A1000 I/O interrupt controller.
>> + *
>> + * Copyright (C) 2021 Loongson Technology Corporation Limited
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/loongarch/loongarch.h"
>> +#include "hw/irq.h"
>> +#include "hw/intc/loongarch_pch_pic.h"
>> +#include "migration/vmstate.h"
>> +#include "trace.h"
>> +
>> +#define for_each_set_bit(bit, addr, size) \
>> +         for ((bit) = find_first_bit((addr), (size));            \
>> +              (bit) < (size);                                    \
>> +              (bit) = find_next_bit((addr), (size), (bit) + 1))
>> +
>> +static void pch_pic_update_irq(loongarch_pch_pic *s, uint64_t mask, int level)
>> +{
>> +    int i;
>> +    uint64_t val;
>> +    val = mask & s->intirr & (~s->int_mask);
>> +
>> +    for_each_set_bit(i, &val, 64) {
>> +        if (level == 1) {
>> +            if ((s->intisr & (0x1ULL << i)) == 0) {
>> +                s->intisr |= 1ULL << i;
>> +                qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], 1);
>> +            }
>> +        } else if (level == 0) {
>> +            if (s->intisr & (0x1ULL << i)) {
>> +                s->intisr &= ~(0x1ULL << i);
>> +                qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], 0);
>> +            }
>> +        }
>> +    }
>> +}
> 
> The normal pattern would be to use something like:
> 
> for (i = 0; i < 64; i++) {
>     if (level) {
>         s->intisr |= 1ULL << i;
>     } else {
>         s->intisr &= ~(0x1ULL << i);
>     }
> 
>     qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], level);
> }
> 
> Why is it necessary to check the previous value of (s->intisr & (0x1ULL << i)) here?

Here check the previous value to avoid Unnecessary write. It seems make things more complicated. I will modify

> 
>> +static void pch_pic_irq_handler(void *opaque, int irq, int level)
>> +{
>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
>> +
>> +    assert(irq < PCH_PIC_IRQ_NUM);
>> +    uint64_t mask = 1ULL << irq;
>> +
>> +    trace_pch_pic_irq_handler(s->intedge, irq, level);
>> +
>> +    if (s->intedge & mask) {
>> +        /* Edge triggered */
>> +        if (level) {
>> +            if ((s->last_intirr & mask) == 0) {
>> +                s->intirr |= mask;
>> +            }
>> +            s->last_intirr |= mask;
>> +        } else {
>> +            s->last_intirr &= ~mask;
>> +        }
>> +    } else {
>> +        /* Level triggered */
>> +        if (level) {
>> +            s->intirr |= mask;
>> +            s->last_intirr |= mask;
>> +        } else {
>> +            s->intirr &= ~mask;
>> +            s->last_intirr &= ~mask;
>> +        }
>> +
>> +    }
>> +    pch_pic_update_irq(s, mask, level);
>> +}
>> +
>> +static uint64_t loongarch_pch_pic_reg_read(void *opaque, hwaddr addr,
>> +                                           unsigned size)
>> +{
>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
>> +    uint64_t val = 0;
>> +    uint32_t offset = addr & 0xfff;
>> +    int64_t offset_tmp;
>> +
>> +    if (size == 8) {
>> +        switch (offset) {
>> +        case PCH_PIC_INT_ID_OFFSET:
>> +            val = (PCH_PIC_INT_ID_NUM << 32) | PCH_PIC_INT_ID_VAL;
>> +            break;
>> +        case PCH_PIC_INT_MASK_OFFSET:
>> +            val =  s->int_mask;
>> +            break;
>> +        case PCH_PIC_INT_STATUS_OFFSET:
>> +            val = s->intisr & (~s->int_mask);
>> +            break;
>> +        case PCH_PIC_INT_EDGE_OFFSET:
>> +            val = s->intedge;
>> +            break;
>> +        case PCH_PIC_INT_POL_OFFSET:
>> +            val = s->int_polarity;
>> +            break;
>> +        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
>> +            val = s->htmsi_en;
>> +            break;
>> +        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
>> +        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
>> +            break;
>> +        default:
>> +            break;
>> +        }
>> +    } else if (size == 4) {
>> +        switch (offset) {
>> +        case PCH_PIC_INT_ID_OFFSET:
>> +            val = PCH_PIC_INT_ID_VAL;
>> +            break;
>> +        case PCH_PIC_INT_ID_OFFSET + 4:
>> +            val = PCH_PIC_INT_ID_NUM;
>> +            break;
>> +        case PCH_PIC_INT_MASK_OFFSET...PCH_PIC_INT_MASK_END:
>> +            val = ldl_p((void *)&s->int_mask +
>> +                        (offset - PCH_PIC_INT_MASK_OFFSET));
>> +            break;
>> +        case PCH_PIC_INT_STATUS_OFFSET...PCH_PIC_INT_STATUS_END:
>> +            val = ldl_p((void *)&s->intisr +
>> +                        (offset - PCH_PIC_INT_STATUS_OFFSET)) & (~s->int_mask);
>> +            break;
>> +        case PCH_PIC_INT_EDGE_OFFSET...PCH_PIC_INT_EDGE_END:
>> +            val = ldl_p((void *)&s->intedge +
>> +                        (offset - PCH_PIC_INT_EDGE_OFFSET));
>> +            break;
>> +        case PCH_PIC_INT_POL_OFFSET...PCH_PIC_INT_POL_END:
>> +            val = ldl_p((void *)&s->int_polarity +
>> +                        (offset - PCH_PIC_INT_POL_OFFSET));
>> +            break;
>> +        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
>> +            val = ldl_p((void *)&s->htmsi_en +
>> +                        (offset - PCH_PIC_HTMSI_EN_OFFSET));
>> +            break;
>> +        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
>> +        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
>> +            break;
>> +        default:
>> +            break;
>> +        }
>> +    } else if (size == 1) {
>> +        if (offset >= PCH_PIC_HTMSI_VEC_OFFSET) {
>> +            offset_tmp = offset - PCH_PIC_HTMSI_VEC_OFFSET;
>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>> +                val = s->htmsi_vector[offset_tmp];
>> +            }
>> +        } else if (offset >=  PCH_PIC_ROUTE_ENTRY_OFFSET) {
>> +            offset_tmp = offset - PCH_PIC_ROUTE_ENTRY_OFFSET;
>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>> +                val = s->route_entry[offset_tmp];
>> +            }
>> +        }
>> +    }
>> +
>> +    trace_loongarch_pch_pic_read(size, (uint32_t)addr, val);
>> +    return val;
>> +}
>> +
>> +static void loongarch_pch_pic_reg_write(void *opaque, hwaddr addr,
>> +                                        uint64_t data, unsigned size)
>> +{
>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
> 
> Perhaps use LoongArchPCHPIC for the camel case version?

OK, all interrupt controller will use the camel case. 

> 
>> +    int32_t offset_tmp;
>> +    uint32_t offset, old;
>> +    offset = addr & 0xfff;
>> +
>> +    trace_loongarch_pch_pic_write(size, (uint32_t)addr, data);
>> +
>> +    if (size == 8) {
>> +        switch (offset) {
>> +        case PCH_PIC_INT_MASK_OFFSET:
>> +            old = s->int_mask;
>> +            s->int_mask = data;
>> +            if (old & ~data) {
>> +                pch_pic_update_irq(s, (old & ~data), 1);
>> +            } else if (~old & data) {
>> +                pch_pic_update_irq(s, (~old & data), 0);
>> +            }
>> +            break;
>> +        case PCH_PIC_INT_STATUS_OFFSET:
>> +            s->intisr = data;
>> +            break;
>> +        case PCH_PIC_INT_EDGE_OFFSET:
>> +            s->intedge = data;
>> +            break;
>> +        case PCH_PIC_INT_CLEAR_OFFSET:
>> +            s->intirr &= (~(data & s->intedge));
>> +            pch_pic_update_irq(s, data, 0);
>> +            s->intisr &= (~data);
>> +            break;
>> +        case PCH_PIC_INT_POL_OFFSET:
>> +            s->int_polarity = data;
>> +            break;
>> +        case PCH_PIC_HTMSI_EN_OFFSET:
>> +            s->htmsi_en = data;
>> +            break;
>> +        case PCH_PIC_AUTO_CTRL0_OFFSET:
>> +        case PCH_PIC_AUTO_CTRL1_OFFSET:
>> +            break;
>> +        default:
>> +            break;
>> +        }
>> +    } else if (size == 4) {
>> +        switch (offset) {
>> +        case PCH_PIC_INT_MASK_OFFSET...PCH_PIC_INT_MASK_END:
>> +            offset -= PCH_PIC_INT_MASK_OFFSET;
>> +            old = ldl_p((void *)&s->int_mask + offset);
>> +            stl_p((void *)&s->int_mask + offset, data);
>> +
>> +            if (old & ~data) {
>> +                pch_pic_update_irq(s, (old & ~data), 1);
>> +            } else if (~old & data) {
>> +                pch_pic_update_irq(s, (~old & data), 0);
>> +            }
>> +            break;
>> +        case PCH_PIC_INT_STATUS_OFFSET...PCH_PIC_INT_STATUS_END:
>> +            stl_p((void *)&s->intisr + (offset - PCH_PIC_INT_STATUS_OFFSET),
>> +                  data);
>> +            break;
>> +        case PCH_PIC_INT_EDGE_OFFSET...PCH_PIC_INT_EDGE_END:
>> +            stl_p((void *)&s->intedge + (offset - PCH_PIC_INT_EDGE_OFFSET),
>> +                  data);
>> +            break;
>> +        case PCH_PIC_INT_CLEAR_OFFSET...PCH_PIC_INT_CLEAR_END:
>> +            old = s->intirr & (~(data & s->intedge));
>> +            stl_p((void *)&s->intirr + (offset - PCH_PIC_INT_CLEAR_OFFSET),
>> +                  old);
>> +            pch_pic_update_irq(s, data, 0);
>> +            old = s->intisr & (~data);
>> +            stl_p((void *)&s->intisr + (offset - PCH_PIC_INT_CLEAR_OFFSET),
>> +                  old);
>> +            break;
>> +        case PCH_PIC_INT_POL_OFFSET...PCH_PIC_INT_POL_END:
>> +            stl_p((void *)&s->int_polarity + (offset - PCH_PIC_INT_POL_OFFSET),
>> +                  data);
>> +            break;
>> +        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
>> +            stl_p((void *)&s->htmsi_en + (offset - PCH_PIC_HTMSI_EN_OFFSET),
>> +                  data);
>> +            break;
>> +        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
>> +        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
>> +            break;
>> +        default:
>> +            break;
>> +        }
>> +    } else if (size == 1) {
>> +        if (offset >= PCH_PIC_HTMSI_VEC_OFFSET) {
>> +            offset_tmp = offset - PCH_PIC_HTMSI_VEC_OFFSET;
>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>> +                s->htmsi_vector[offset_tmp] = (uint8_t)(data & 0xff);
>> +            }
>> +        } else if (offset >=  PCH_PIC_ROUTE_ENTRY_OFFSET) {
>> +            offset_tmp = offset - PCH_PIC_ROUTE_ENTRY_OFFSET;
>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>> +                s->route_entry[offset_tmp] = (uint8_t)(data & 0xff);
>> +            }
>> +        }
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps loongarch_pch_pic_ops = {
>> +    .read = loongarch_pch_pic_reg_read,
>> +    .write = loongarch_pch_pic_reg_write,
>> +    .valid = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 8,
>> +    },
>> +    .impl = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 8,
>> +    },
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
> 
> It feels like this should be split into 2 separate memory regions: as per before set .min_access_size = 4 for PCH_PIC_INT_MASK_OFFSET ... PCH_PIC_AUTO_CTRL1_END and use uint32_t for the underlying types in loongarch_pch_pic.
> 
> This leaves a separate memory region for accesses > PCH_PIC_HTMSI_VEC_OFFSET which can be set with .min_access_size = 1 which maps nicely to the underlying uint8_t types. Which means you should be able to access the arrays directly instead of having to use ldl_p() and stl_p().
> 

yeah, here the PCH_PIC_HTMSI_VEC_OFFSET region is right in the middle, I want to use a separate memory region to overlap the pch-pic region, is it ok to do like this? Thanks

> +static void loongarch_pch_pic_reset(DeviceState *d)
>> +{
>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(d);
>> +    int i;
>> +
>> +    s->int_mask = -1ULL;
>> +    s->htmsi_en = 0x0;
>> +    s->intedge  = 0x0;
>> +    s->intclr   = 0x0;
>> +    s->auto_crtl0 = 0x0;
>> +    s->auto_crtl1 = 0x0;
>> +    for (i = 0; i < 64; i++) {
>> +        s->route_entry[i] = 0x1;
>> +        s->htmsi_vector[i] = 0x0;
>> +    }
>> +    s->intirr = 0x0;
>> +    s->intisr = 0x0;
>> +    s->last_intirr = 0x0;
>> +    s->int_polarity = 0x0;
>> +}
>> +
>> +static void loongarch_pch_pic_init(Object *obj)
>> +{
>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(obj);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> +    int i;
>> +
>> +    memory_region_init_io(&s->iomem, obj, &loongarch_pch_pic_ops,
>> +                          s, TYPE_LOONGARCH_PCH_PIC, 0x1000);
>> +    sysbus_init_mmio(sbd, &s->iomem);
>> +
>> +    for (i = 0; i < PCH_PIC_IRQ_NUM; i++) {
>> +        sysbus_init_irq(sbd, &s->parent_irq[i]);
>> +    }
>> +    qdev_init_gpio_in(DEVICE(obj), pch_pic_irq_handler, PCH_PIC_IRQ_NUM);
>> +}
>> +
>> +static const VMStateDescription vmstate_loongarch_pch_pic = {
>> +    .name = TYPE_LOONGARCH_PCH_PIC,
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT64(int_mask, loongarch_pch_pic),
>> +        VMSTATE_UINT64(htmsi_en, loongarch_pch_pic),
>> +        VMSTATE_UINT64(intedge, loongarch_pch_pic),
>> +        VMSTATE_UINT64(intclr, loongarch_pch_pic),
>> +        VMSTATE_UINT64(auto_crtl0, loongarch_pch_pic),
>> +        VMSTATE_UINT64(auto_crtl1, loongarch_pch_pic),
>> +        VMSTATE_UINT8_ARRAY(route_entry, loongarch_pch_pic, 64),
>> +        VMSTATE_UINT8_ARRAY(htmsi_vector, loongarch_pch_pic, 64),
>> +        VMSTATE_UINT64(last_intirr, loongarch_pch_pic),
>> +        VMSTATE_UINT64(intirr, loongarch_pch_pic),
>> +        VMSTATE_UINT64(intisr, loongarch_pch_pic),
>> +        VMSTATE_UINT64(int_polarity, loongarch_pch_pic),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static void loongarch_pch_pic_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->reset = loongarch_pch_pic_reset;
>> +    dc->vmsd = &vmstate_loongarch_pch_pic;
>> +}
>> +
>> +static const TypeInfo loongarch_pch_pic_info = {
>> +    .name          = TYPE_LOONGARCH_PCH_PIC,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(loongarch_pch_pic),
>> +    .instance_init = loongarch_pch_pic_init,
>> +    .class_init    = loongarch_pch_pic_class_init,
>> +};
>> +
>> +static void loongarch_pch_pic_register_types(void)
>> +{
>> +    type_register_static(&loongarch_pch_pic_info);
>> +}
>> +
>> +type_init(loongarch_pch_pic_register_types)
>> diff --git a/hw/intc/meson.build b/hw/intc/meson.build
>> index 51f0c3988a..33ba63266e 100644
>> --- a/hw/intc/meson.build
>> +++ b/hw/intc/meson.build
>> @@ -58,3 +58,4 @@ specific_ss.add(when: ['CONFIG_KVM', 'CONFIG_XIVE'],
>>   specific_ss.add(when: 'CONFIG_GOLDFISH_PIC', if_true: files('goldfish_pic.c'))
>>   specific_ss.add(when: 'CONFIG_M68K_IRQC', if_true: files('m68k_irqc.c'))
>>   specific_ss.add(when: 'CONFIG_LOONGARCH_IPI', if_true: files('loongarch_ipi.c'))
>> +specific_ss.add(when: 'CONFIG_LOONGARCH_PCH_PIC', if_true: files('loongarch_pch_pic.c'))
>> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
>> index 124608e51f..52fedf82be 100644
>> --- a/hw/intc/trace-events
>> +++ b/hw/intc/trace-events
>> @@ -250,3 +250,8 @@ sh_intc_set(int id, int enable) "setting interrupt group %d to %d"
>>   # loongarch_ipi.c
>>   loongarch_ipi_read(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%"PRIx64
>>   loongarch_ipi_write(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%"PRIx64
>> +
>> +# loongarch_pch_pic.c
>> +pch_pic_irq_handler(uint32_t edge, int irq, int level) "edge 0x%02x irq %d level %d"
>> +loongarch_pch_pic_read(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx64
>> +loongarch_pch_pic_write(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx64
>> diff --git a/hw/loongarch/Kconfig b/hw/loongarch/Kconfig
>> index 1591574397..c2b8046b94 100644
>> --- a/hw/loongarch/Kconfig
>> +++ b/hw/loongarch/Kconfig
>> @@ -2,3 +2,4 @@ config LOONGSON3_LS7A
>>       bool
>>       select PCI_EXPRESS_7A
>>       select LOONGARCH_IPI
>> +    select LOONGARCH_PCH_PIC
>> diff --git a/include/hw/intc/loongarch_pch_pic.h b/include/hw/intc/loongarch_pch_pic.h
>> new file mode 100644
>> index 0000000000..bc04ed28ef
>> --- /dev/null
>> +++ b/include/hw/intc/loongarch_pch_pic.h
>> @@ -0,0 +1,61 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * LoongArch 7A1000 I/O interrupt controller definitions
>> + *
>> + * Copyright (c) 2021 Loongson Technology Corporation Limited
>> + */
>> +
>> +#define TYPE_LOONGARCH_PCH_PIC "loongarch_pch_pic"
>> +DECLARE_INSTANCE_CHECKER(struct loongarch_pch_pic, LOONGARCH_PCH_PIC,
>> +                         TYPE_LOONGARCH_PCH_PIC)
>> +
>> +#define PCH_PIC_IRQ_START               0
>> +#define PCH_PIC_IRQ_END                 63
>> +#define PCH_PIC_IRQ_NUM                 64
>> +#define PCH_PIC_INT_ID_VAL              0x7000000UL
>> +#define PCH_PIC_INT_ID_NUM              0x3f0001UL
>> +
>> +#define PCH_PIC_INT_ID_OFFSET           0x00
>> +#define PCH_PIC_INT_ID_END              0x07
>> +#define PCH_PIC_INT_MASK_OFFSET         0x20
>> +#define PCH_PIC_INT_MASK_END            0x27
>> +#define PCH_PIC_HTMSI_EN_OFFSET         0x40
>> +#define PCH_PIC_HTMSI_EN_END            0x47
>> +#define PCH_PIC_INT_EDGE_OFFSET         0x60
>> +#define PCH_PIC_INT_EDGE_END            0x67
>> +#define PCH_PIC_INT_CLEAR_OFFSET        0x80
>> +#define PCH_PIC_INT_CLEAR_END           0x87
>> +#define PCH_PIC_AUTO_CTRL0_OFFSET       0xc0
>> +#define PCH_PIC_AUTO_CTRL0_END          0xc7
>> +#define PCH_PIC_AUTO_CTRL1_OFFSET       0xe0
>> +#define PCH_PIC_AUTO_CTRL1_END          0xe8
>> +#define PCH_PIC_ROUTE_ENTRY_OFFSET      0x100
>> +#define PCH_PIC_ROUTE_ENTRY_END         0x13f
>> +#define PCH_PIC_HTMSI_VEC_OFFSET        0x200
>> +#define PCH_PIC_HTMSI_VEC_END           0x23f
>> +#define PCH_PIC_INT_STATUS_OFFSET       0x3a0
>> +#define PCH_PIC_INT_STATUS_END          0x3a7
>> +#define PCH_PIC_INT_POL_OFFSET          0x3e0
>> +#define PCH_PIC_INT_POL_END             0x3e7
>> +
>> +typedef struct loongarch_pch_pic {
>> +    SysBusDevice parent_obj;
>> +    qemu_irq parent_irq[64];
>> +    uint64_t int_mask; /*0x020 interrupt mask register*/
>> +    uint64_t htmsi_en;/*0x040 1=msi*/
>> +    uint64_t intedge; /*0x060 edge=1 level  =0*/
>> +    uint64_t intclr; /*0x080 for clean edge int,set 1 clean,set 0 is noused*/
>> +    uint64_t auto_crtl0; /*0x0c0*/
>> +    uint64_t auto_crtl1; /*0x0e0*/
>> +    uint8_t route_entry[64]; /*0x100 - 0x138*/
>> +    uint8_t htmsi_vector[64]; /*0x200 - 0x238*/
>> +    uint64_t last_intirr;    /* edge detection */
>> +    uint64_t intirr; /* 0x380 interrupt request register */
>> +    uint64_t intisr; /* 0x3a0 interrupt service register */
>> +    /*
>> +     * 0x3e0 interrupt level polarity selection
>> +     * register 0 for high level trigger
>> +     */
>> +    uint64_t int_polarity;
>> +    MemoryRegion iomem;
>> +} loongarch_pch_pic;
> 
> 
> ATB,
> 
> Mark.

Re: [RFC PATCH v3 18/27] hw/intc: Add LoongArch ls7a interrupt controller support(PCH-PIC)

Posted by Mark Cave-Ayland 3 years, 12 months ago

On 22/12/2021 02:38, yangxiaojuan wrote:

> Hi, Mark
> 
> On 12/18/2021 08:33 AM, Mark Cave-Ayland wrote:
>> On 04/12/2021 12:07, Xiaojuan Yang wrote:
>>
>>> This patch realize the PCH-PIC interrupt controller.
>>>
>>> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
>>> Signed-off-by: Song Gao <gaosong@loongson.cn>
>>> ---
>>>    hw/intc/Kconfig                     |   4 +
>>>    hw/intc/loongarch_pch_pic.c         | 357 ++++++++++++++++++++++++++++
>>>    hw/intc/meson.build                 |   1 +
>>>    hw/intc/trace-events                |   5 +
>>>    hw/loongarch/Kconfig                |   1 +
>>>    include/hw/intc/loongarch_pch_pic.h |  61 +++++
>>>    6 files changed, 429 insertions(+)
>>>    create mode 100644 hw/intc/loongarch_pch_pic.c
>>>    create mode 100644 include/hw/intc/loongarch_pch_pic.h
>>>
>>> diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
>>> index 511dcac537..96da13ad1d 100644
>>> --- a/hw/intc/Kconfig
>>> +++ b/hw/intc/Kconfig
>>> @@ -76,3 +76,7 @@ config M68K_IRQC
>>>      config LOONGARCH_IPI
>>>        bool
>>> +
>>> +config LOONGARCH_PCH_PIC
>>> +    bool
>>> +    select UNIMP
>>> diff --git a/hw/intc/loongarch_pch_pic.c b/hw/intc/loongarch_pch_pic.c
>>> new file mode 100644
>>> index 0000000000..2ede29ceb0
>>> --- /dev/null
>>> +++ b/hw/intc/loongarch_pch_pic.c
>>> @@ -0,0 +1,357 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * QEMU Loongson 7A1000 I/O interrupt controller.
>>> + *
>>> + * Copyright (C) 2021 Loongson Technology Corporation Limited
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "hw/sysbus.h"
>>> +#include "hw/loongarch/loongarch.h"
>>> +#include "hw/irq.h"
>>> +#include "hw/intc/loongarch_pch_pic.h"
>>> +#include "migration/vmstate.h"
>>> +#include "trace.h"
>>> +
>>> +#define for_each_set_bit(bit, addr, size) \
>>> +         for ((bit) = find_first_bit((addr), (size));            \
>>> +              (bit) < (size);                                    \
>>> +              (bit) = find_next_bit((addr), (size), (bit) + 1))
>>> +
>>> +static void pch_pic_update_irq(loongarch_pch_pic *s, uint64_t mask, int level)
>>> +{
>>> +    int i;
>>> +    uint64_t val;
>>> +    val = mask & s->intirr & (~s->int_mask);
>>> +
>>> +    for_each_set_bit(i, &val, 64) {
>>> +        if (level == 1) {
>>> +            if ((s->intisr & (0x1ULL << i)) == 0) {
>>> +                s->intisr |= 1ULL << i;
>>> +                qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], 1);
>>> +            }
>>> +        } else if (level == 0) {
>>> +            if (s->intisr & (0x1ULL << i)) {
>>> +                s->intisr &= ~(0x1ULL << i);
>>> +                qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], 0);
>>> +            }
>>> +        }
>>> +    }
>>> +}
>>
>> The normal pattern would be to use something like:
>>
>> for (i = 0; i < 64; i++) {
>>      if (level) {
>>          s->intisr |= 1ULL << i;
>>      } else {
>>          s->intisr &= ~(0x1ULL << i);
>>      }
>>
>>      qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], level);
>> }
>>
>> Why is it necessary to check the previous value of (s->intisr & (0x1ULL << i)) here?
> 
> Here check the previous value to avoid Unnecessary write. It seems make things more complicated. I will modify

In general a *_update_irq() function should be fine to propagate the IRQ up to the 
parent directly: I think this is fine in this case because you are directly 
manipulating the parent_irq elements rather than using e.g. a priority encoder within 
this device to raise an IRQ to the CPU. I'm presuming this final prioritisation and 
delivery is done elsewhere?

>>> +static void pch_pic_irq_handler(void *opaque, int irq, int level)
>>> +{
>>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
>>> +
>>> +    assert(irq < PCH_PIC_IRQ_NUM);
>>> +    uint64_t mask = 1ULL << irq;
>>> +
>>> +    trace_pch_pic_irq_handler(s->intedge, irq, level);
>>> +
>>> +    if (s->intedge & mask) {
>>> +        /* Edge triggered */
>>> +        if (level) {
>>> +            if ((s->last_intirr & mask) == 0) {
>>> +                s->intirr |= mask;
>>> +            }
>>> +            s->last_intirr |= mask;
>>> +        } else {
>>> +            s->last_intirr &= ~mask;
>>> +        }
>>> +    } else {
>>> +        /* Level triggered */
>>> +        if (level) {
>>> +            s->intirr |= mask;
>>> +            s->last_intirr |= mask;
>>> +        } else {
>>> +            s->intirr &= ~mask;
>>> +            s->last_intirr &= ~mask;
>>> +        }
>>> +
>>> +    }
>>> +    pch_pic_update_irq(s, mask, level);
>>> +}
>>> +
>>> +static uint64_t loongarch_pch_pic_reg_read(void *opaque, hwaddr addr,
>>> +                                           unsigned size)
>>> +{
>>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
>>> +    uint64_t val = 0;
>>> +    uint32_t offset = addr & 0xfff;
>>> +    int64_t offset_tmp;
>>> +
>>> +    if (size == 8) {
>>> +        switch (offset) {
>>> +        case PCH_PIC_INT_ID_OFFSET:
>>> +            val = (PCH_PIC_INT_ID_NUM << 32) | PCH_PIC_INT_ID_VAL;
>>> +            break;
>>> +        case PCH_PIC_INT_MASK_OFFSET:
>>> +            val =  s->int_mask;
>>> +            break;
>>> +        case PCH_PIC_INT_STATUS_OFFSET:
>>> +            val = s->intisr & (~s->int_mask);
>>> +            break;
>>> +        case PCH_PIC_INT_EDGE_OFFSET:
>>> +            val = s->intedge;
>>> +            break;
>>> +        case PCH_PIC_INT_POL_OFFSET:
>>> +            val = s->int_polarity;
>>> +            break;
>>> +        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
>>> +            val = s->htmsi_en;
>>> +            break;
>>> +        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
>>> +        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
>>> +            break;
>>> +        default:
>>> +            break;
>>> +        }
>>> +    } else if (size == 4) {
>>> +        switch (offset) {
>>> +        case PCH_PIC_INT_ID_OFFSET:
>>> +            val = PCH_PIC_INT_ID_VAL;
>>> +            break;
>>> +        case PCH_PIC_INT_ID_OFFSET + 4:
>>> +            val = PCH_PIC_INT_ID_NUM;
>>> +            break;
>>> +        case PCH_PIC_INT_MASK_OFFSET...PCH_PIC_INT_MASK_END:
>>> +            val = ldl_p((void *)&s->int_mask +
>>> +                        (offset - PCH_PIC_INT_MASK_OFFSET));
>>> +            break;
>>> +        case PCH_PIC_INT_STATUS_OFFSET...PCH_PIC_INT_STATUS_END:
>>> +            val = ldl_p((void *)&s->intisr +
>>> +                        (offset - PCH_PIC_INT_STATUS_OFFSET)) & (~s->int_mask);
>>> +            break;
>>> +        case PCH_PIC_INT_EDGE_OFFSET...PCH_PIC_INT_EDGE_END:
>>> +            val = ldl_p((void *)&s->intedge +
>>> +                        (offset - PCH_PIC_INT_EDGE_OFFSET));
>>> +            break;
>>> +        case PCH_PIC_INT_POL_OFFSET...PCH_PIC_INT_POL_END:
>>> +            val = ldl_p((void *)&s->int_polarity +
>>> +                        (offset - PCH_PIC_INT_POL_OFFSET));
>>> +            break;
>>> +        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
>>> +            val = ldl_p((void *)&s->htmsi_en +
>>> +                        (offset - PCH_PIC_HTMSI_EN_OFFSET));
>>> +            break;
>>> +        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
>>> +        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
>>> +            break;
>>> +        default:
>>> +            break;
>>> +        }
>>> +    } else if (size == 1) {
>>> +        if (offset >= PCH_PIC_HTMSI_VEC_OFFSET) {
>>> +            offset_tmp = offset - PCH_PIC_HTMSI_VEC_OFFSET;
>>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>>> +                val = s->htmsi_vector[offset_tmp];
>>> +            }
>>> +        } else if (offset >=  PCH_PIC_ROUTE_ENTRY_OFFSET) {
>>> +            offset_tmp = offset - PCH_PIC_ROUTE_ENTRY_OFFSET;
>>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>>> +                val = s->route_entry[offset_tmp];
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +    trace_loongarch_pch_pic_read(size, (uint32_t)addr, val);
>>> +    return val;
>>> +}
>>> +
>>> +static void loongarch_pch_pic_reg_write(void *opaque, hwaddr addr,
>>> +                                        uint64_t data, unsigned size)
>>> +{
>>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
>>
>> Perhaps use LoongArchPCHPIC for the camel case version?
> 
> OK, all interrupt controller will use the camel case.
> 
>>
>>> +    int32_t offset_tmp;
>>> +    uint32_t offset, old;
>>> +    offset = addr & 0xfff;
>>> +
>>> +    trace_loongarch_pch_pic_write(size, (uint32_t)addr, data);
>>> +
>>> +    if (size == 8) {
>>> +        switch (offset) {
>>> +        case PCH_PIC_INT_MASK_OFFSET:
>>> +            old = s->int_mask;
>>> +            s->int_mask = data;
>>> +            if (old & ~data) {
>>> +                pch_pic_update_irq(s, (old & ~data), 1);
>>> +            } else if (~old & data) {
>>> +                pch_pic_update_irq(s, (~old & data), 0);
>>> +            }
>>> +            break;
>>> +        case PCH_PIC_INT_STATUS_OFFSET:
>>> +            s->intisr = data;
>>> +            break;
>>> +        case PCH_PIC_INT_EDGE_OFFSET:
>>> +            s->intedge = data;
>>> +            break;
>>> +        case PCH_PIC_INT_CLEAR_OFFSET:
>>> +            s->intirr &= (~(data & s->intedge));
>>> +            pch_pic_update_irq(s, data, 0);
>>> +            s->intisr &= (~data);
>>> +            break;
>>> +        case PCH_PIC_INT_POL_OFFSET:
>>> +            s->int_polarity = data;
>>> +            break;
>>> +        case PCH_PIC_HTMSI_EN_OFFSET:
>>> +            s->htmsi_en = data;
>>> +            break;
>>> +        case PCH_PIC_AUTO_CTRL0_OFFSET:
>>> +        case PCH_PIC_AUTO_CTRL1_OFFSET:
>>> +            break;
>>> +        default:
>>> +            break;
>>> +        }
>>> +    } else if (size == 4) {
>>> +        switch (offset) {
>>> +        case PCH_PIC_INT_MASK_OFFSET...PCH_PIC_INT_MASK_END:
>>> +            offset -= PCH_PIC_INT_MASK_OFFSET;
>>> +            old = ldl_p((void *)&s->int_mask + offset);
>>> +            stl_p((void *)&s->int_mask + offset, data);
>>> +
>>> +            if (old & ~data) {
>>> +                pch_pic_update_irq(s, (old & ~data), 1);
>>> +            } else if (~old & data) {
>>> +                pch_pic_update_irq(s, (~old & data), 0);
>>> +            }
>>> +            break;
>>> +        case PCH_PIC_INT_STATUS_OFFSET...PCH_PIC_INT_STATUS_END:
>>> +            stl_p((void *)&s->intisr + (offset - PCH_PIC_INT_STATUS_OFFSET),
>>> +                  data);
>>> +            break;
>>> +        case PCH_PIC_INT_EDGE_OFFSET...PCH_PIC_INT_EDGE_END:
>>> +            stl_p((void *)&s->intedge + (offset - PCH_PIC_INT_EDGE_OFFSET),
>>> +                  data);
>>> +            break;
>>> +        case PCH_PIC_INT_CLEAR_OFFSET...PCH_PIC_INT_CLEAR_END:
>>> +            old = s->intirr & (~(data & s->intedge));
>>> +            stl_p((void *)&s->intirr + (offset - PCH_PIC_INT_CLEAR_OFFSET),
>>> +                  old);
>>> +            pch_pic_update_irq(s, data, 0);
>>> +            old = s->intisr & (~data);
>>> +            stl_p((void *)&s->intisr + (offset - PCH_PIC_INT_CLEAR_OFFSET),
>>> +                  old);
>>> +            break;
>>> +        case PCH_PIC_INT_POL_OFFSET...PCH_PIC_INT_POL_END:
>>> +            stl_p((void *)&s->int_polarity + (offset - PCH_PIC_INT_POL_OFFSET),
>>> +                  data);
>>> +            break;
>>> +        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
>>> +            stl_p((void *)&s->htmsi_en + (offset - PCH_PIC_HTMSI_EN_OFFSET),
>>> +                  data);
>>> +            break;
>>> +        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
>>> +        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
>>> +            break;
>>> +        default:
>>> +            break;
>>> +        }
>>> +    } else if (size == 1) {
>>> +        if (offset >= PCH_PIC_HTMSI_VEC_OFFSET) {
>>> +            offset_tmp = offset - PCH_PIC_HTMSI_VEC_OFFSET;
>>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>>> +                s->htmsi_vector[offset_tmp] = (uint8_t)(data & 0xff);
>>> +            }
>>> +        } else if (offset >=  PCH_PIC_ROUTE_ENTRY_OFFSET) {
>>> +            offset_tmp = offset - PCH_PIC_ROUTE_ENTRY_OFFSET;
>>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>>> +                s->route_entry[offset_tmp] = (uint8_t)(data & 0xff);
>>> +            }
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +static const MemoryRegionOps loongarch_pch_pic_ops = {
>>> +    .read = loongarch_pch_pic_reg_read,
>>> +    .write = loongarch_pch_pic_reg_write,
>>> +    .valid = {
>>> +        .min_access_size = 1,
>>> +        .max_access_size = 8,
>>> +    },
>>> +    .impl = {
>>> +        .min_access_size = 1,
>>> +        .max_access_size = 8,
>>> +    },
>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>> +};
>>
>> It feels like this should be split into 2 separate memory regions: as per before set .min_access_size = 4 for PCH_PIC_INT_MASK_OFFSET ... PCH_PIC_AUTO_CTRL1_END and use uint32_t for the underlying types in loongarch_pch_pic.
>>
>> This leaves a separate memory region for accesses > PCH_PIC_HTMSI_VEC_OFFSET which can be set with .min_access_size = 1 which maps nicely to the underlying uint8_t types. Which means you should be able to access the arrays directly instead of having to use ldl_p() and stl_p().
>>
> 
> yeah, here the PCH_PIC_HTMSI_VEC_OFFSET region is right in the middle, I want to use a separate memory region to overlap the pch-pic region, is it ok to do like this? Thanks

That should be fine. I'd also use a single container memory region so that you only 
have to expose a single memory region with sysbus_init_mmio() as below:

>> +static void loongarch_pch_pic_reset(DeviceState *d)
>>> +{
>>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(d);
>>> +    int i;
>>> +
>>> +    s->int_mask = -1ULL;
>>> +    s->htmsi_en = 0x0;
>>> +    s->intedge  = 0x0;
>>> +    s->intclr   = 0x0;
>>> +    s->auto_crtl0 = 0x0;
>>> +    s->auto_crtl1 = 0x0;
>>> +    for (i = 0; i < 64; i++) {
>>> +        s->route_entry[i] = 0x1;
>>> +        s->htmsi_vector[i] = 0x0;
>>> +    }
>>> +    s->intirr = 0x0;
>>> +    s->intisr = 0x0;
>>> +    s->last_intirr = 0x0;
>>> +    s->int_polarity = 0x0;
>>> +}
>>> +
>>> +static void loongarch_pch_pic_init(Object *obj)
>>> +{
>>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(obj);
>>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>>> +    int i;
>>> +
>>> +    memory_region_init_io(&s->iomem, obj, &loongarch_pch_pic_ops,
>>> +                          s, TYPE_LOONGARCH_PCH_PIC, 0x1000);
>>> +    sysbus_init_mmio(sbd, &s->iomem);

Please adjust the example below for your particular case replacing the sizes as 
appropriate but I would expect it to look something like:

     memory_region_init(&s->picmem, obj, TYPE_LOONGARCH_PCH_PIC, 0x1000);

     memory_region_init_io(&s->picintmem, obj, &loongarch_pch_pic_int_ops,
                           s, "pic.int", 0xaaaa);
     memory_region_add_subregion(&s->picmem, INT_OFFSET, &s->picintmem);

     memory_region_init_io(&s->picintmem, obj, &loongarch_pch_pic_htmsi_ops,
                           s, "pic.htmsi", 0xbbbb);
     memory_region_add_subregion(&s->picmem, HTMSI_OFFSET, &s->picintmem);

     memory_region_init_io(&s->picroutemem, obj, &loongarch_pch_pic_route_ops,
                           s, "pic.route", 0xcccc);
     memory_region_add_subregion(&s->picmem, ROUTE_OFFSET, &s->picroutemem);

     sysbus_init_mmio(sbd, &s->picmem);

You can then configure each of the *_ops memory regions with the correct .min.access 
and .max.access sizes accordingly, and each region will show up separately in the 
output of "info mtree".

>>> +    for (i = 0; i < PCH_PIC_IRQ_NUM; i++) {
>>> +        sysbus_init_irq(sbd, &s->parent_irq[i]);
>>> +    }
>>> +    qdev_init_gpio_in(DEVICE(obj), pch_pic_irq_handler, PCH_PIC_IRQ_NUM);
>>> +}
>>> +
>>> +static const VMStateDescription vmstate_loongarch_pch_pic = {
>>> +    .name = TYPE_LOONGARCH_PCH_PIC,
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_UINT64(int_mask, loongarch_pch_pic),
>>> +        VMSTATE_UINT64(htmsi_en, loongarch_pch_pic),
>>> +        VMSTATE_UINT64(intedge, loongarch_pch_pic),
>>> +        VMSTATE_UINT64(intclr, loongarch_pch_pic),
>>> +        VMSTATE_UINT64(auto_crtl0, loongarch_pch_pic),
>>> +        VMSTATE_UINT64(auto_crtl1, loongarch_pch_pic),
>>> +        VMSTATE_UINT8_ARRAY(route_entry, loongarch_pch_pic, 64),
>>> +        VMSTATE_UINT8_ARRAY(htmsi_vector, loongarch_pch_pic, 64),
>>> +        VMSTATE_UINT64(last_intirr, loongarch_pch_pic),
>>> +        VMSTATE_UINT64(intirr, loongarch_pch_pic),
>>> +        VMSTATE_UINT64(intisr, loongarch_pch_pic),
>>> +        VMSTATE_UINT64(int_polarity, loongarch_pch_pic),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>> +static void loongarch_pch_pic_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +
>>> +    dc->reset = loongarch_pch_pic_reset;
>>> +    dc->vmsd = &vmstate_loongarch_pch_pic;
>>> +}
>>> +
>>> +static const TypeInfo loongarch_pch_pic_info = {
>>> +    .name          = TYPE_LOONGARCH_PCH_PIC,
>>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>>> +    .instance_size = sizeof(loongarch_pch_pic),
>>> +    .instance_init = loongarch_pch_pic_init,
>>> +    .class_init    = loongarch_pch_pic_class_init,
>>> +};
>>> +
>>> +static void loongarch_pch_pic_register_types(void)
>>> +{
>>> +    type_register_static(&loongarch_pch_pic_info);
>>> +}
>>> +
>>> +type_init(loongarch_pch_pic_register_types)
>>> diff --git a/hw/intc/meson.build b/hw/intc/meson.build
>>> index 51f0c3988a..33ba63266e 100644
>>> --- a/hw/intc/meson.build
>>> +++ b/hw/intc/meson.build
>>> @@ -58,3 +58,4 @@ specific_ss.add(when: ['CONFIG_KVM', 'CONFIG_XIVE'],
>>>    specific_ss.add(when: 'CONFIG_GOLDFISH_PIC', if_true: files('goldfish_pic.c'))
>>>    specific_ss.add(when: 'CONFIG_M68K_IRQC', if_true: files('m68k_irqc.c'))
>>>    specific_ss.add(when: 'CONFIG_LOONGARCH_IPI', if_true: files('loongarch_ipi.c'))
>>> +specific_ss.add(when: 'CONFIG_LOONGARCH_PCH_PIC', if_true: files('loongarch_pch_pic.c'))
>>> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
>>> index 124608e51f..52fedf82be 100644
>>> --- a/hw/intc/trace-events
>>> +++ b/hw/intc/trace-events
>>> @@ -250,3 +250,8 @@ sh_intc_set(int id, int enable) "setting interrupt group %d to %d"
>>>    # loongarch_ipi.c
>>>    loongarch_ipi_read(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%"PRIx64
>>>    loongarch_ipi_write(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%"PRIx64
>>> +
>>> +# loongarch_pch_pic.c
>>> +pch_pic_irq_handler(uint32_t edge, int irq, int level) "edge 0x%02x irq %d level %d"
>>> +loongarch_pch_pic_read(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx64
>>> +loongarch_pch_pic_write(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx64
>>> diff --git a/hw/loongarch/Kconfig b/hw/loongarch/Kconfig
>>> index 1591574397..c2b8046b94 100644
>>> --- a/hw/loongarch/Kconfig
>>> +++ b/hw/loongarch/Kconfig
>>> @@ -2,3 +2,4 @@ config LOONGSON3_LS7A
>>>        bool
>>>        select PCI_EXPRESS_7A
>>>        select LOONGARCH_IPI
>>> +    select LOONGARCH_PCH_PIC
>>> diff --git a/include/hw/intc/loongarch_pch_pic.h b/include/hw/intc/loongarch_pch_pic.h
>>> new file mode 100644
>>> index 0000000000..bc04ed28ef
>>> --- /dev/null
>>> +++ b/include/hw/intc/loongarch_pch_pic.h
>>> @@ -0,0 +1,61 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * LoongArch 7A1000 I/O interrupt controller definitions
>>> + *
>>> + * Copyright (c) 2021 Loongson Technology Corporation Limited
>>> + */
>>> +
>>> +#define TYPE_LOONGARCH_PCH_PIC "loongarch_pch_pic"
>>> +DECLARE_INSTANCE_CHECKER(struct loongarch_pch_pic, LOONGARCH_PCH_PIC,
>>> +                         TYPE_LOONGARCH_PCH_PIC)
>>> +
>>> +#define PCH_PIC_IRQ_START               0
>>> +#define PCH_PIC_IRQ_END                 63
>>> +#define PCH_PIC_IRQ_NUM                 64
>>> +#define PCH_PIC_INT_ID_VAL              0x7000000UL
>>> +#define PCH_PIC_INT_ID_NUM              0x3f0001UL
>>> +
>>> +#define PCH_PIC_INT_ID_OFFSET           0x00
>>> +#define PCH_PIC_INT_ID_END              0x07
>>> +#define PCH_PIC_INT_MASK_OFFSET         0x20
>>> +#define PCH_PIC_INT_MASK_END            0x27
>>> +#define PCH_PIC_HTMSI_EN_OFFSET         0x40
>>> +#define PCH_PIC_HTMSI_EN_END            0x47
>>> +#define PCH_PIC_INT_EDGE_OFFSET         0x60
>>> +#define PCH_PIC_INT_EDGE_END            0x67
>>> +#define PCH_PIC_INT_CLEAR_OFFSET        0x80
>>> +#define PCH_PIC_INT_CLEAR_END           0x87
>>> +#define PCH_PIC_AUTO_CTRL0_OFFSET       0xc0
>>> +#define PCH_PIC_AUTO_CTRL0_END          0xc7
>>> +#define PCH_PIC_AUTO_CTRL1_OFFSET       0xe0
>>> +#define PCH_PIC_AUTO_CTRL1_END          0xe8
>>> +#define PCH_PIC_ROUTE_ENTRY_OFFSET      0x100
>>> +#define PCH_PIC_ROUTE_ENTRY_END         0x13f
>>> +#define PCH_PIC_HTMSI_VEC_OFFSET        0x200
>>> +#define PCH_PIC_HTMSI_VEC_END           0x23f
>>> +#define PCH_PIC_INT_STATUS_OFFSET       0x3a0
>>> +#define PCH_PIC_INT_STATUS_END          0x3a7
>>> +#define PCH_PIC_INT_POL_OFFSET          0x3e0
>>> +#define PCH_PIC_INT_POL_END             0x3e7
>>> +
>>> +typedef struct loongarch_pch_pic {
>>> +    SysBusDevice parent_obj;
>>> +    qemu_irq parent_irq[64];
>>> +    uint64_t int_mask; /*0x020 interrupt mask register*/
>>> +    uint64_t htmsi_en;/*0x040 1=msi*/
>>> +    uint64_t intedge; /*0x060 edge=1 level  =0*/
>>> +    uint64_t intclr; /*0x080 for clean edge int,set 1 clean,set 0 is noused*/
>>> +    uint64_t auto_crtl0; /*0x0c0*/
>>> +    uint64_t auto_crtl1; /*0x0e0*/
>>> +    uint8_t route_entry[64]; /*0x100 - 0x138*/
>>> +    uint8_t htmsi_vector[64]; /*0x200 - 0x238*/
>>> +    uint64_t last_intirr;    /* edge detection */
>>> +    uint64_t intirr; /* 0x380 interrupt request register */
>>> +    uint64_t intisr; /* 0x3a0 interrupt service register */
>>> +    /*
>>> +     * 0x3e0 interrupt level polarity selection
>>> +     * register 0 for high level trigger
>>> +     */
>>> +    uint64_t int_polarity;
>>> +    MemoryRegion iomem;
>>> +} loongarch_pch_pic;

ATB,

Mark.

Re: [RFC PATCH v3 18/27] hw/intc: Add LoongArch ls7a interrupt controller support(PCH-PIC)

Posted by yangxiaojuan 3 years, 11 months ago

Hi,Mark:

  Sorry for the late reply. I just saw the mail after I send the v4 patch. I sorted the mail into different folders from
the qemu-devel, so I didn't see the mail in time. Sorry again.

Xiaojuan

On 12/23/2021 06:21 PM, Mark Cave-Ayland wrote:
> On 22/12/2021 02:38, yangxiaojuan wrote:
> 
>> Hi, Mark
>>
>> On 12/18/2021 08:33 AM, Mark Cave-Ayland wrote:
>>> On 04/12/2021 12:07, Xiaojuan Yang wrote:
>>>
>>>> This patch realize the PCH-PIC interrupt controller.
>>>>
>>>> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
>>>> Signed-off-by: Song Gao <gaosong@loongson.cn>
>>>> ---
>>>>    hw/intc/Kconfig                     |   4 +
>>>>    hw/intc/loongarch_pch_pic.c         | 357 ++++++++++++++++++++++++++++
>>>>    hw/intc/meson.build                 |   1 +
>>>>    hw/intc/trace-events                |   5 +
>>>>    hw/loongarch/Kconfig                |   1 +
>>>>    include/hw/intc/loongarch_pch_pic.h |  61 +++++
>>>>    6 files changed, 429 insertions(+)
>>>>    create mode 100644 hw/intc/loongarch_pch_pic.c
>>>>    create mode 100644 include/hw/intc/loongarch_pch_pic.h
>>>>
>>>> diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
>>>> index 511dcac537..96da13ad1d 100644
>>>> --- a/hw/intc/Kconfig
>>>> +++ b/hw/intc/Kconfig
>>>> @@ -76,3 +76,7 @@ config M68K_IRQC
>>>>      config LOONGARCH_IPI
>>>>        bool
>>>> +
>>>> +config LOONGARCH_PCH_PIC
>>>> +    bool
>>>> +    select UNIMP
>>>> diff --git a/hw/intc/loongarch_pch_pic.c b/hw/intc/loongarch_pch_pic.c
>>>> new file mode 100644
>>>> index 0000000000..2ede29ceb0
>>>> --- /dev/null
>>>> +++ b/hw/intc/loongarch_pch_pic.c
>>>> @@ -0,0 +1,357 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +/*
>>>> + * QEMU Loongson 7A1000 I/O interrupt controller.
>>>> + *
>>>> + * Copyright (C) 2021 Loongson Technology Corporation Limited
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "hw/sysbus.h"
>>>> +#include "hw/loongarch/loongarch.h"
>>>> +#include "hw/irq.h"
>>>> +#include "hw/intc/loongarch_pch_pic.h"
>>>> +#include "migration/vmstate.h"
>>>> +#include "trace.h"
>>>> +
>>>> +#define for_each_set_bit(bit, addr, size) \
>>>> +         for ((bit) = find_first_bit((addr), (size));            \
>>>> +              (bit) < (size);                                    \
>>>> +              (bit) = find_next_bit((addr), (size), (bit) + 1))
>>>> +
>>>> +static void pch_pic_update_irq(loongarch_pch_pic *s, uint64_t mask, int level)
>>>> +{
>>>> +    int i;
>>>> +    uint64_t val;
>>>> +    val = mask & s->intirr & (~s->int_mask);
>>>> +
>>>> +    for_each_set_bit(i, &val, 64) {
>>>> +        if (level == 1) {
>>>> +            if ((s->intisr & (0x1ULL << i)) == 0) {
>>>> +                s->intisr |= 1ULL << i;
>>>> +                qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], 1);
>>>> +            }
>>>> +        } else if (level == 0) {
>>>> +            if (s->intisr & (0x1ULL << i)) {
>>>> +                s->intisr &= ~(0x1ULL << i);
>>>> +                qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], 0);
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +}
>>>
>>> The normal pattern would be to use something like:
>>>
>>> for (i = 0; i < 64; i++) {
>>>      if (level) {
>>>          s->intisr |= 1ULL << i;
>>>      } else {
>>>          s->intisr &= ~(0x1ULL << i);
>>>      }
>>>
>>>      qemu_set_irq(s->parent_irq[s->htmsi_vector[i]], level);
>>> }
>>>
>>> Why is it necessary to check the previous value of (s->intisr & (0x1ULL << i)) here?
>>
>> Here check the previous value to avoid Unnecessary write. It seems make things more complicated. I will modify
> 
> In general a *_update_irq() function should be fine to propagate the IRQ up to the parent directly: I think this is fine in this case because you are directly manipulating the parent_irq elements rather than using e.g. a priority encoder within this device to raise an IRQ to the CPU. I'm presuming this final prioritisation and delivery is done elsewhere?
> 
>>>> +static void pch_pic_irq_handler(void *opaque, int irq, int level)
>>>> +{
>>>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
>>>> +
>>>> +    assert(irq < PCH_PIC_IRQ_NUM);
>>>> +    uint64_t mask = 1ULL << irq;
>>>> +
>>>> +    trace_pch_pic_irq_handler(s->intedge, irq, level);
>>>> +
>>>> +    if (s->intedge & mask) {
>>>> +        /* Edge triggered */
>>>> +        if (level) {
>>>> +            if ((s->last_intirr & mask) == 0) {
>>>> +                s->intirr |= mask;
>>>> +            }
>>>> +            s->last_intirr |= mask;
>>>> +        } else {
>>>> +            s->last_intirr &= ~mask;
>>>> +        }
>>>> +    } else {
>>>> +        /* Level triggered */
>>>> +        if (level) {
>>>> +            s->intirr |= mask;
>>>> +            s->last_intirr |= mask;
>>>> +        } else {
>>>> +            s->intirr &= ~mask;
>>>> +            s->last_intirr &= ~mask;
>>>> +        }
>>>> +
>>>> +    }
>>>> +    pch_pic_update_irq(s, mask, level);
>>>> +}
>>>> +
>>>> +static uint64_t loongarch_pch_pic_reg_read(void *opaque, hwaddr addr,
>>>> +                                           unsigned size)
>>>> +{
>>>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
>>>> +    uint64_t val = 0;
>>>> +    uint32_t offset = addr & 0xfff;
>>>> +    int64_t offset_tmp;
>>>> +
>>>> +    if (size == 8) {
>>>> +        switch (offset) {
>>>> +        case PCH_PIC_INT_ID_OFFSET:
>>>> +            val = (PCH_PIC_INT_ID_NUM << 32) | PCH_PIC_INT_ID_VAL;
>>>> +            break;
>>>> +        case PCH_PIC_INT_MASK_OFFSET:
>>>> +            val =  s->int_mask;
>>>> +            break;
>>>> +        case PCH_PIC_INT_STATUS_OFFSET:
>>>> +            val = s->intisr & (~s->int_mask);
>>>> +            break;
>>>> +        case PCH_PIC_INT_EDGE_OFFSET:
>>>> +            val = s->intedge;
>>>> +            break;
>>>> +        case PCH_PIC_INT_POL_OFFSET:
>>>> +            val = s->int_polarity;
>>>> +            break;
>>>> +        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
>>>> +            val = s->htmsi_en;
>>>> +            break;
>>>> +        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
>>>> +        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
>>>> +            break;
>>>> +        default:
>>>> +            break;
>>>> +        }
>>>> +    } else if (size == 4) {
>>>> +        switch (offset) {
>>>> +        case PCH_PIC_INT_ID_OFFSET:
>>>> +            val = PCH_PIC_INT_ID_VAL;
>>>> +            break;
>>>> +        case PCH_PIC_INT_ID_OFFSET + 4:
>>>> +            val = PCH_PIC_INT_ID_NUM;
>>>> +            break;
>>>> +        case PCH_PIC_INT_MASK_OFFSET...PCH_PIC_INT_MASK_END:
>>>> +            val = ldl_p((void *)&s->int_mask +
>>>> +                        (offset - PCH_PIC_INT_MASK_OFFSET));
>>>> +            break;
>>>> +        case PCH_PIC_INT_STATUS_OFFSET...PCH_PIC_INT_STATUS_END:
>>>> +            val = ldl_p((void *)&s->intisr +
>>>> +                        (offset - PCH_PIC_INT_STATUS_OFFSET)) & (~s->int_mask);
>>>> +            break;
>>>> +        case PCH_PIC_INT_EDGE_OFFSET...PCH_PIC_INT_EDGE_END:
>>>> +            val = ldl_p((void *)&s->intedge +
>>>> +                        (offset - PCH_PIC_INT_EDGE_OFFSET));
>>>> +            break;
>>>> +        case PCH_PIC_INT_POL_OFFSET...PCH_PIC_INT_POL_END:
>>>> +            val = ldl_p((void *)&s->int_polarity +
>>>> +                        (offset - PCH_PIC_INT_POL_OFFSET));
>>>> +            break;
>>>> +        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
>>>> +            val = ldl_p((void *)&s->htmsi_en +
>>>> +                        (offset - PCH_PIC_HTMSI_EN_OFFSET));
>>>> +            break;
>>>> +        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
>>>> +        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
>>>> +            break;
>>>> +        default:
>>>> +            break;
>>>> +        }
>>>> +    } else if (size == 1) {
>>>> +        if (offset >= PCH_PIC_HTMSI_VEC_OFFSET) {
>>>> +            offset_tmp = offset - PCH_PIC_HTMSI_VEC_OFFSET;
>>>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>>>> +                val = s->htmsi_vector[offset_tmp];
>>>> +            }
>>>> +        } else if (offset >=  PCH_PIC_ROUTE_ENTRY_OFFSET) {
>>>> +            offset_tmp = offset - PCH_PIC_ROUTE_ENTRY_OFFSET;
>>>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>>>> +                val = s->route_entry[offset_tmp];
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +
>>>> +    trace_loongarch_pch_pic_read(size, (uint32_t)addr, val);
>>>> +    return val;
>>>> +}
>>>> +
>>>> +static void loongarch_pch_pic_reg_write(void *opaque, hwaddr addr,
>>>> +                                        uint64_t data, unsigned size)
>>>> +{
>>>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(opaque);
>>>
>>> Perhaps use LoongArchPCHPIC for the camel case version?
>>
>> OK, all interrupt controller will use the camel case.
>>
>>>
>>>> +    int32_t offset_tmp;
>>>> +    uint32_t offset, old;
>>>> +    offset = addr & 0xfff;
>>>> +
>>>> +    trace_loongarch_pch_pic_write(size, (uint32_t)addr, data);
>>>> +
>>>> +    if (size == 8) {
>>>> +        switch (offset) {
>>>> +        case PCH_PIC_INT_MASK_OFFSET:
>>>> +            old = s->int_mask;
>>>> +            s->int_mask = data;
>>>> +            if (old & ~data) {
>>>> +                pch_pic_update_irq(s, (old & ~data), 1);
>>>> +            } else if (~old & data) {
>>>> +                pch_pic_update_irq(s, (~old & data), 0);
>>>> +            }
>>>> +            break;
>>>> +        case PCH_PIC_INT_STATUS_OFFSET:
>>>> +            s->intisr = data;
>>>> +            break;
>>>> +        case PCH_PIC_INT_EDGE_OFFSET:
>>>> +            s->intedge = data;
>>>> +            break;
>>>> +        case PCH_PIC_INT_CLEAR_OFFSET:
>>>> +            s->intirr &= (~(data & s->intedge));
>>>> +            pch_pic_update_irq(s, data, 0);
>>>> +            s->intisr &= (~data);
>>>> +            break;
>>>> +        case PCH_PIC_INT_POL_OFFSET:
>>>> +            s->int_polarity = data;
>>>> +            break;
>>>> +        case PCH_PIC_HTMSI_EN_OFFSET:
>>>> +            s->htmsi_en = data;
>>>> +            break;
>>>> +        case PCH_PIC_AUTO_CTRL0_OFFSET:
>>>> +        case PCH_PIC_AUTO_CTRL1_OFFSET:
>>>> +            break;
>>>> +        default:
>>>> +            break;
>>>> +        }
>>>> +    } else if (size == 4) {
>>>> +        switch (offset) {
>>>> +        case PCH_PIC_INT_MASK_OFFSET...PCH_PIC_INT_MASK_END:
>>>> +            offset -= PCH_PIC_INT_MASK_OFFSET;
>>>> +            old = ldl_p((void *)&s->int_mask + offset);
>>>> +            stl_p((void *)&s->int_mask + offset, data);
>>>> +
>>>> +            if (old & ~data) {
>>>> +                pch_pic_update_irq(s, (old & ~data), 1);
>>>> +            } else if (~old & data) {
>>>> +                pch_pic_update_irq(s, (~old & data), 0);
>>>> +            }
>>>> +            break;
>>>> +        case PCH_PIC_INT_STATUS_OFFSET...PCH_PIC_INT_STATUS_END:
>>>> +            stl_p((void *)&s->intisr + (offset - PCH_PIC_INT_STATUS_OFFSET),
>>>> +                  data);
>>>> +            break;
>>>> +        case PCH_PIC_INT_EDGE_OFFSET...PCH_PIC_INT_EDGE_END:
>>>> +            stl_p((void *)&s->intedge + (offset - PCH_PIC_INT_EDGE_OFFSET),
>>>> +                  data);
>>>> +            break;
>>>> +        case PCH_PIC_INT_CLEAR_OFFSET...PCH_PIC_INT_CLEAR_END:
>>>> +            old = s->intirr & (~(data & s->intedge));
>>>> +            stl_p((void *)&s->intirr + (offset - PCH_PIC_INT_CLEAR_OFFSET),
>>>> +                  old);
>>>> +            pch_pic_update_irq(s, data, 0);
>>>> +            old = s->intisr & (~data);
>>>> +            stl_p((void *)&s->intisr + (offset - PCH_PIC_INT_CLEAR_OFFSET),
>>>> +                  old);
>>>> +            break;
>>>> +        case PCH_PIC_INT_POL_OFFSET...PCH_PIC_INT_POL_END:
>>>> +            stl_p((void *)&s->int_polarity + (offset - PCH_PIC_INT_POL_OFFSET),
>>>> +                  data);
>>>> +            break;
>>>> +        case PCH_PIC_HTMSI_EN_OFFSET...PCH_PIC_HTMSI_EN_END:
>>>> +            stl_p((void *)&s->htmsi_en + (offset - PCH_PIC_HTMSI_EN_OFFSET),
>>>> +                  data);
>>>> +            break;
>>>> +        case PCH_PIC_AUTO_CTRL0_OFFSET...PCH_PIC_AUTO_CTRL0_END:
>>>> +        case PCH_PIC_AUTO_CTRL1_OFFSET...PCH_PIC_AUTO_CTRL1_END:
>>>> +            break;
>>>> +        default:
>>>> +            break;
>>>> +        }
>>>> +    } else if (size == 1) {
>>>> +        if (offset >= PCH_PIC_HTMSI_VEC_OFFSET) {
>>>> +            offset_tmp = offset - PCH_PIC_HTMSI_VEC_OFFSET;
>>>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>>>> +                s->htmsi_vector[offset_tmp] = (uint8_t)(data & 0xff);
>>>> +            }
>>>> +        } else if (offset >=  PCH_PIC_ROUTE_ENTRY_OFFSET) {
>>>> +            offset_tmp = offset - PCH_PIC_ROUTE_ENTRY_OFFSET;
>>>> +            if (offset_tmp >= 0 && offset_tmp < 64) {
>>>> +                s->route_entry[offset_tmp] = (uint8_t)(data & 0xff);
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>> +static const MemoryRegionOps loongarch_pch_pic_ops = {
>>>> +    .read = loongarch_pch_pic_reg_read,
>>>> +    .write = loongarch_pch_pic_reg_write,
>>>> +    .valid = {
>>>> +        .min_access_size = 1,
>>>> +        .max_access_size = 8,
>>>> +    },
>>>> +    .impl = {
>>>> +        .min_access_size = 1,
>>>> +        .max_access_size = 8,
>>>> +    },
>>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>>> +};
>>>
>>> It feels like this should be split into 2 separate memory regions: as per before set .min_access_size = 4 for PCH_PIC_INT_MASK_OFFSET ... PCH_PIC_AUTO_CTRL1_END and use uint32_t for the underlying types in loongarch_pch_pic.
>>>
>>> This leaves a separate memory region for accesses > PCH_PIC_HTMSI_VEC_OFFSET which can be set with .min_access_size = 1 which maps nicely to the underlying uint8_t types. Which means you should be able to access the arrays directly instead of having to use ldl_p() and stl_p().
>>>
>>
>> yeah, here the PCH_PIC_HTMSI_VEC_OFFSET region is right in the middle, I want to use a separate memory region to overlap the pch-pic region, is it ok to do like this? Thanks
> 
> That should be fine. I'd also use a single container memory region so that you only have to expose a single memory region with sysbus_init_mmio() as below:
> 
>>> +static void loongarch_pch_pic_reset(DeviceState *d)
>>>> +{
>>>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(d);
>>>> +    int i;
>>>> +
>>>> +    s->int_mask = -1ULL;
>>>> +    s->htmsi_en = 0x0;
>>>> +    s->intedge  = 0x0;
>>>> +    s->intclr   = 0x0;
>>>> +    s->auto_crtl0 = 0x0;
>>>> +    s->auto_crtl1 = 0x0;
>>>> +    for (i = 0; i < 64; i++) {
>>>> +        s->route_entry[i] = 0x1;
>>>> +        s->htmsi_vector[i] = 0x0;
>>>> +    }
>>>> +    s->intirr = 0x0;
>>>> +    s->intisr = 0x0;
>>>> +    s->last_intirr = 0x0;
>>>> +    s->int_polarity = 0x0;
>>>> +}
>>>> +
>>>> +static void loongarch_pch_pic_init(Object *obj)
>>>> +{
>>>> +    loongarch_pch_pic *s = LOONGARCH_PCH_PIC(obj);
>>>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>>>> +    int i;
>>>> +
>>>> +    memory_region_init_io(&s->iomem, obj, &loongarch_pch_pic_ops,
>>>> +                          s, TYPE_LOONGARCH_PCH_PIC, 0x1000);
>>>> +    sysbus_init_mmio(sbd, &s->iomem);
> 
> Please adjust the example below for your particular case replacing the sizes as appropriate but I would expect it to look something like:
> 
>     memory_region_init(&s->picmem, obj, TYPE_LOONGARCH_PCH_PIC, 0x1000);
> 
>     memory_region_init_io(&s->picintmem, obj, &loongarch_pch_pic_int_ops,
>                           s, "pic.int", 0xaaaa);
>     memory_region_add_subregion(&s->picmem, INT_OFFSET, &s->picintmem);
> 
>     memory_region_init_io(&s->picintmem, obj, &loongarch_pch_pic_htmsi_ops,
>                           s, "pic.htmsi", 0xbbbb);
>     memory_region_add_subregion(&s->picmem, HTMSI_OFFSET, &s->picintmem);
> 
>     memory_region_init_io(&s->picroutemem, obj, &loongarch_pch_pic_route_ops,
>                           s, "pic.route", 0xcccc);
>     memory_region_add_subregion(&s->picmem, ROUTE_OFFSET, &s->picroutemem);
> 
>     sysbus_init_mmio(sbd, &s->picmem);
> 
> You can then configure each of the *_ops memory regions with the correct .min.access and .max.access sizes accordingly, and each region will show up separately in the output of "info mtree".
> 
>>>> +    for (i = 0; i < PCH_PIC_IRQ_NUM; i++) {
>>>> +        sysbus_init_irq(sbd, &s->parent_irq[i]);
>>>> +    }
>>>> +    qdev_init_gpio_in(DEVICE(obj), pch_pic_irq_handler, PCH_PIC_IRQ_NUM);
>>>> +}
>>>> +
>>>> +static const VMStateDescription vmstate_loongarch_pch_pic = {
>>>> +    .name = TYPE_LOONGARCH_PCH_PIC,
>>>> +    .version_id = 1,
>>>> +    .minimum_version_id = 1,
>>>> +    .fields = (VMStateField[]) {
>>>> +        VMSTATE_UINT64(int_mask, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(htmsi_en, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(intedge, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(intclr, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(auto_crtl0, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(auto_crtl1, loongarch_pch_pic),
>>>> +        VMSTATE_UINT8_ARRAY(route_entry, loongarch_pch_pic, 64),
>>>> +        VMSTATE_UINT8_ARRAY(htmsi_vector, loongarch_pch_pic, 64),
>>>> +        VMSTATE_UINT64(last_intirr, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(intirr, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(intisr, loongarch_pch_pic),
>>>> +        VMSTATE_UINT64(int_polarity, loongarch_pch_pic),
>>>> +        VMSTATE_END_OF_LIST()
>>>> +    }
>>>> +};
>>>> +
>>>> +static void loongarch_pch_pic_class_init(ObjectClass *klass, void *data)
>>>> +{
>>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>>> +
>>>> +    dc->reset = loongarch_pch_pic_reset;
>>>> +    dc->vmsd = &vmstate_loongarch_pch_pic;
>>>> +}
>>>> +
>>>> +static const TypeInfo loongarch_pch_pic_info = {
>>>> +    .name          = TYPE_LOONGARCH_PCH_PIC,
>>>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>>>> +    .instance_size = sizeof(loongarch_pch_pic),
>>>> +    .instance_init = loongarch_pch_pic_init,
>>>> +    .class_init    = loongarch_pch_pic_class_init,
>>>> +};
>>>> +
>>>> +static void loongarch_pch_pic_register_types(void)
>>>> +{
>>>> +    type_register_static(&loongarch_pch_pic_info);
>>>> +}
>>>> +
>>>> +type_init(loongarch_pch_pic_register_types)
>>>> diff --git a/hw/intc/meson.build b/hw/intc/meson.build
>>>> index 51f0c3988a..33ba63266e 100644
>>>> --- a/hw/intc/meson.build
>>>> +++ b/hw/intc/meson.build
>>>> @@ -58,3 +58,4 @@ specific_ss.add(when: ['CONFIG_KVM', 'CONFIG_XIVE'],
>>>>    specific_ss.add(when: 'CONFIG_GOLDFISH_PIC', if_true: files('goldfish_pic.c'))
>>>>    specific_ss.add(when: 'CONFIG_M68K_IRQC', if_true: files('m68k_irqc.c'))
>>>>    specific_ss.add(when: 'CONFIG_LOONGARCH_IPI', if_true: files('loongarch_ipi.c'))
>>>> +specific_ss.add(when: 'CONFIG_LOONGARCH_PCH_PIC', if_true: files('loongarch_pch_pic.c'))
>>>> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
>>>> index 124608e51f..52fedf82be 100644
>>>> --- a/hw/intc/trace-events
>>>> +++ b/hw/intc/trace-events
>>>> @@ -250,3 +250,8 @@ sh_intc_set(int id, int enable) "setting interrupt group %d to %d"
>>>>    # loongarch_ipi.c
>>>>    loongarch_ipi_read(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%"PRIx64
>>>>    loongarch_ipi_write(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%"PRIx64
>>>> +
>>>> +# loongarch_pch_pic.c
>>>> +pch_pic_irq_handler(uint32_t edge, int irq, int level) "edge 0x%02x irq %d level %d"
>>>> +loongarch_pch_pic_read(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx64
>>>> +loongarch_pch_pic_write(unsigned size, uint32_t addr, unsigned long val) "size: %u addr: 0x%"PRIx32 "val: 0x%" PRIx64
>>>> diff --git a/hw/loongarch/Kconfig b/hw/loongarch/Kconfig
>>>> index 1591574397..c2b8046b94 100644
>>>> --- a/hw/loongarch/Kconfig
>>>> +++ b/hw/loongarch/Kconfig
>>>> @@ -2,3 +2,4 @@ config LOONGSON3_LS7A
>>>>        bool
>>>>        select PCI_EXPRESS_7A
>>>>        select LOONGARCH_IPI
>>>> +    select LOONGARCH_PCH_PIC
>>>> diff --git a/include/hw/intc/loongarch_pch_pic.h b/include/hw/intc/loongarch_pch_pic.h
>>>> new file mode 100644
>>>> index 0000000000..bc04ed28ef
>>>> --- /dev/null
>>>> +++ b/include/hw/intc/loongarch_pch_pic.h
>>>> @@ -0,0 +1,61 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +/*
>>>> + * LoongArch 7A1000 I/O interrupt controller definitions
>>>> + *
>>>> + * Copyright (c) 2021 Loongson Technology Corporation Limited
>>>> + */
>>>> +
>>>> +#define TYPE_LOONGARCH_PCH_PIC "loongarch_pch_pic"
>>>> +DECLARE_INSTANCE_CHECKER(struct loongarch_pch_pic, LOONGARCH_PCH_PIC,
>>>> +                         TYPE_LOONGARCH_PCH_PIC)
>>>> +
>>>> +#define PCH_PIC_IRQ_START               0
>>>> +#define PCH_PIC_IRQ_END                 63
>>>> +#define PCH_PIC_IRQ_NUM                 64
>>>> +#define PCH_PIC_INT_ID_VAL              0x7000000UL
>>>> +#define PCH_PIC_INT_ID_NUM              0x3f0001UL
>>>> +
>>>> +#define PCH_PIC_INT_ID_OFFSET           0x00
>>>> +#define PCH_PIC_INT_ID_END              0x07
>>>> +#define PCH_PIC_INT_MASK_OFFSET         0x20
>>>> +#define PCH_PIC_INT_MASK_END            0x27
>>>> +#define PCH_PIC_HTMSI_EN_OFFSET         0x40
>>>> +#define PCH_PIC_HTMSI_EN_END            0x47
>>>> +#define PCH_PIC_INT_EDGE_OFFSET         0x60
>>>> +#define PCH_PIC_INT_EDGE_END            0x67
>>>> +#define PCH_PIC_INT_CLEAR_OFFSET        0x80
>>>> +#define PCH_PIC_INT_CLEAR_END           0x87
>>>> +#define PCH_PIC_AUTO_CTRL0_OFFSET       0xc0
>>>> +#define PCH_PIC_AUTO_CTRL0_END          0xc7
>>>> +#define PCH_PIC_AUTO_CTRL1_OFFSET       0xe0
>>>> +#define PCH_PIC_AUTO_CTRL1_END          0xe8
>>>> +#define PCH_PIC_ROUTE_ENTRY_OFFSET      0x100
>>>> +#define PCH_PIC_ROUTE_ENTRY_END         0x13f
>>>> +#define PCH_PIC_HTMSI_VEC_OFFSET        0x200
>>>> +#define PCH_PIC_HTMSI_VEC_END           0x23f
>>>> +#define PCH_PIC_INT_STATUS_OFFSET       0x3a0
>>>> +#define PCH_PIC_INT_STATUS_END          0x3a7
>>>> +#define PCH_PIC_INT_POL_OFFSET          0x3e0
>>>> +#define PCH_PIC_INT_POL_END             0x3e7
>>>> +
>>>> +typedef struct loongarch_pch_pic {
>>>> +    SysBusDevice parent_obj;
>>>> +    qemu_irq parent_irq[64];
>>>> +    uint64_t int_mask; /*0x020 interrupt mask register*/
>>>> +    uint64_t htmsi_en;/*0x040 1=msi*/
>>>> +    uint64_t intedge; /*0x060 edge=1 level  =0*/
>>>> +    uint64_t intclr; /*0x080 for clean edge int,set 1 clean,set 0 is noused*/
>>>> +    uint64_t auto_crtl0; /*0x0c0*/
>>>> +    uint64_t auto_crtl1; /*0x0e0*/
>>>> +    uint8_t route_entry[64]; /*0x100 - 0x138*/
>>>> +    uint8_t htmsi_vector[64]; /*0x200 - 0x238*/
>>>> +    uint64_t last_intirr;    /* edge detection */
>>>> +    uint64_t intirr; /* 0x380 interrupt request register */
>>>> +    uint64_t intisr; /* 0x3a0 interrupt service register */
>>>> +    /*
>>>> +     * 0x3e0 interrupt level polarity selection
>>>> +     * register 0 for high level trigger
>>>> +     */
>>>> +    uint64_t int_polarity;
>>>> +    MemoryRegion iomem;
>>>> +} loongarch_pch_pic;
> 
> ATB,
> 
> Mark.