ast irq controller needs to support gen 5 boards

[shenki]

the ast2500 has a different number of IRQs, so the masks in the emulated hardware are incorrect.

https://github.com/openbmc/qemu/blob/master/hw/intc/aspeed_vic.c#L38

The mask values for gen 5 socs are

-           valid-sources = <0xffffffff 0x0007ffff>;        
+           valid-sources = <0xfefff7ff 0x0807ffff>;

I also suspect these reset values are incorrect:

https://github.com/openbmc/qemu/blob/master/hw/intc/aspeed_vic.c#L279

[shenki]

ASPEED_VIC_NR_IRQS is incorrect as well. It could just do a popcount of the valid sources instead of hard coding?

[amboar]

ASPEED_VIC_NR_IRQS is used for qdev_init_gpio_in() during initialisation, but we could probably just pass 64 there?

[shenki]

What do you think about this?

From: Joel Stanley <joel@jms.id.au>
Subject: [PATCH] hw/intc/aspeed_vic: Drop hardcoded masks

The hardware has 64 interrupt lines, and in a specific machine they may
be configured as the designer sees fit. Remove the machine-specific
configuration from the model.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 hw/intc/aspeed_vic.c         | 41 ++++++-----------------------------------
 include/hw/intc/aspeed_vic.h |  2 +-
 2 files changed, 7 insertions(+), 36 deletions(-)

diff --git a/hw/intc/aspeed_vic.c b/hw/intc/aspeed_vic.c
index 19a0ff74864e..4bde5792af93 100644
--- a/hw/intc/aspeed_vic.c
+++ b/hw/intc/aspeed_vic.c
@@ -35,10 +35,6 @@

 #define AVIC_NEW_BASE_OFFSET 0x80

-#define AVIC_L_MASK 0xFFFFFFFFU
-#define AVIC_H_MASK 0x0007FFFFU
-#define AVIC_EVENT_W_MASK (0x78000ULL << 32)
-
 static void aspeed_vic_update(AspeedVICState *s)
 {
     uint64_t new = (s->raw & s->enable);
@@ -59,7 +55,8 @@ static void aspeed_vic_set_irq(void *opaque, int irq, int level)
     bool raise;
     AspeedVICState *s = (AspeedVICState *)opaque;

-    if (irq > ASPEED_VIC_NR_IRQS) {
+    /* -1, as irqs start at zero */
+    if (irq > (ASPEED_VIC_NR_IRQS - 1)) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid interrupt number: %d\n",
                       __func__, irq);
         return;
@@ -173,7 +170,6 @@ static uint64_t aspeed_vic_read(void *opaque, hwaddr offset, unsigned size)
 static void aspeed_vic_write(void *opaque, hwaddr offset, uint64_t data,
                              unsigned size)
 {
-    const bool high = !!(offset & 0x4);
     hwaddr n_offset = (offset & ~0x4);
     AspeedVICState *s = (AspeedVICState *)opaque;

@@ -188,25 +184,9 @@ static void aspeed_vic_write(void *opaque, hwaddr offset, uint64_t data,
     n_offset -= AVIC_NEW_BASE_OFFSET;
     trace_aspeed_vic_write(offset, size, data);

-    /* Given we have members using separate enable/clear registers, deposit64()
-     * isn't quite the tool for the job. Instead, relocate the incoming bits to
-     * the required bit offset based on the provided access address
-     */
-    if (high) {
-        data &= AVIC_H_MASK;
-        data <<= 32;
-    } else {
-        data &= AVIC_L_MASK;
-    }
-
     switch (n_offset) {
     case 0x18: /* Interrupt Selection */
         /* Register has deposit64() semantics - overwrite requested 32 bits */
-        if (high) {
-            s->select &= AVIC_L_MASK;
-        } else {
-            s->select &= ((uint64_t) AVIC_H_MASK) << 32;
-        }
         s->select |= data;
         break;
     case 0x20: /* Interrupt Enable */
@@ -224,16 +204,7 @@ static void aspeed_vic_write(void *opaque, hwaddr offset, uint64_t data,
                       "IRQs to be cleared: 0x%016" PRIx64 "\n", __func__, data);
         break;
     case 0x50: /* Interrupt Event */
-        /* Register has deposit64() semantics - overwrite the top four valid
-         * IRQ bits, as only the top four IRQs (GPIOs) can change their event
-         * type */
-        if (high) {
-            s->event &= ~AVIC_EVENT_W_MASK;
-            s->event |= (data & AVIC_EVENT_W_MASK);
-        } else {
-            qemu_log_mask(LOG_GUEST_ERROR,
-                          "Ignoring invalid write to interrupt event register");
-        }
+   s->event |= data;
         break;
     case 0x58: /* Edge Triggered Interrupt Clear */
         s->raw &= ~(data & ~s->sense);
@@ -276,9 +247,9 @@ static void aspeed_vic_reset(DeviceState *dev)
     s->select = 0;
     s->enable = 0;
     s->trigger = 0;
-    s->sense = 0x1F07FFF8FFFFULL;
-    s->dual_edge = 0xF800070000ULL;
-    s->event = 0x5F07FFF8FFFFULL;
+    s->sense = 0;
+    s->dual_edge = 0;
+    s->event = 0;
 }

 #define AVIC_IO_REGION_SIZE 0x20000
diff --git a/include/hw/intc/aspeed_vic.h b/include/hw/intc/aspeed_vic.h
index 107ff17c3be4..fe43f01f83be 100644
--- a/include/hw/intc/aspeed_vic.h
+++ b/include/hw/intc/aspeed_vic.h
@@ -18,7 +18,7 @@
 #define TYPE_ASPEED_VIC "aspeed.vic"
 #define ASPEED_VIC(obj) OBJECT_CHECK(AspeedVICState, (obj), TYPE_ASPEED_VIC)

-#define ASPEED_VIC_NR_IRQS 51
+#define ASPEED_VIC_NR_IRQS 64

 typedef struct AspeedVICState {
     /*< private >*/
-- 
2.7.4

[amboar]

Not convinced about just zeroing the reset values, might have to find another technique for getting the right bits in there?

[shenki]

I added qdev properties for the reset values:

https://github.com/shenki/qemu/commit/d1f117ebadc08a5df1cb6d42711d77c26e12e9e5 https://github.com/shenki/qemu/commit/b9820f7168a00a138043789e2759a69df425d90f

This removes the valid IRQ masks from the model. We have three options there:

• make them qdev properties as well
• make them correspond to the irq lines actually hooked up in the model
• leave them out

[legoater]

This is fixed in latest qemu.