Re: [Qemu-devel] SH: Improve the interrupt controller

[takasi-y]

Hi, Vladimir.

Some more fixes added. Would you please test this for your target?
I would like you to post it if it works for yours.

I've tested 
 Head: svn://svn.sv.gnu.org/qemu/address@hidden
 + Last patch: 
http://lists.gnu.org/archive/html/qemu-devel/2009-02/msg00546.html
 + This patch.
 + "-append" patch: 
http://lists.gnu.org/archive/html/qemu-devel/2009-02/msg00727.html
with r2d target.

> sh7785 emulation for your changes. It works, but only after I re-do the
> INTC_MODE_DUAL_SET/CLR swap form my original patch. I think that's about 
> right.
> Suppose you set a bit in SET register, the interrupt then should be masked 
> out.
This "value" comes form *valuep, that points mask_regs[i].value.
And go back into it at the end of this function as "*valuep = value;".

So, I think it better be handled as MASK value(1:mask 0:enable) same as bits in
 INTMSK registers, but as ENABLE(1:enable 0:mask) value as 
sh_intc_toggle_mask().
That's why 
    case INTC_MODE_DUAL_SET: value |= *valuep; break;
seems to be OK. And then we need to invert the arg for toggle_mask like
    sh_intc_toggle_mask(desc, enum_ids[k], !(value & mask), priority, 0);

> However, unless I miss something, it seems like the value read from a register
> is actually inverted. ...
I guess the fix above solves this, too.

> But probably, it's best if your combined patch is checked in -- as I've said
> I get a working sh4a emulation based on your patch, and it's problematic
> to keep such big patches outside the official tree.
Right. Even though I still think this code itself is somewhat problematic.
I added four more fix in addition to the one above.
Anyway, I think it is OK to be checked in if no serious regression found.

- Set priority before calling toggle_source
 (Important when both enable and priority are set simultaneously). 

- Fix IRL level<->priority conversion.
 Basically, SH's irq levels are 4bit(disable+15level), except INT2PRI(is 5bit).
 This patch canonicalize them to 5bit(2disables+30level), same as Vladimir's.

- Fix enable condition of priority to "priority>1" for 5bit priority.
 Both 0 and 1 indicate "disable" on INT2PRI(5bit priority).

- Add lower/upper limit to enable_count.
 Original code expect mask vs clear or priority=N(N>1) vs priority={0,1}
 are strictly paired. Otherwise it goes into weired state. For example,
  Setting pri=1 enables irq, then
  setting pri=2 disables (!), then
  setting pri=0 enables irq(!!) with level 0(!!!).
 This limits enable_count into [0,enable_max].  Though, I still don't know
 what the original author wanted to realize with this code.
 (I know this type of counter technique is used to enable nested mask operation.
  But, anyway, real HW doesn't have that functionality).

- Fix some indents, spaces, and comment.

/yoshii
---
Improve the interrupt controller. 2nd fix.

Signed-off-by: Takashi YOSHII <address@hidden>
---
 hw/sh_intc.c |   35 +++++++++++++++++++----------------
 hw/sh_intc.h |    4 ++--
 2 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/hw/sh_intc.c b/hw/sh_intc.c
index 7641b11..7cd1fad 100644
--- a/hw/sh_intc.c
+++ b/hw/sh_intc.c
@@ -94,7 +94,7 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int 
*priority)
     /* slow: use a linked list of pending sources instead */
 
     if (*priority == 0x0f)
-        return -1;
+       return -1;
 
     for (i = 0; i < desc->nr_sources; i++) {
         struct intc_source *source = desc->sources + i;
@@ -107,7 +107,7 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int 
*priority)
 
     if (found != -1  && highest_priority > *priority) {
        struct intc_source *source = desc->sources + found;
-       
+
 #ifdef DEBUG_INTC_SOURCES
        printf("sh_intc: (%d) returning interrupt source 0x%x\n",
               desc->pending, source->vect);
@@ -117,7 +117,7 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int 
*priority)
 
        /* Priority in intc is 5 bits, whereas processor knows only 4 bits. */
        *priority = highest_priority >> 1;
-       
+
        return source->vect;
     }
     return -1;
@@ -215,9 +215,12 @@ static void sh_intc_toggle_mask(struct intc_desc *desc, 
intc_enum id,
     }
 
     if (source->vect) {
-        sh_intc_toggle_source(source, enable ? 1 : -1, 0);
-        if (priority != -1)
-            source->priority = priority;
+       if (priority != -1)
+           source->priority = priority;
+       if (enable && source->enable_count < source->enable_max)
+           sh_intc_toggle_source(source, 1, 0);
+       if (!enable && source->enable_count > 0)
+           sh_intc_toggle_source(source, -1, 0);
     }
 
 #ifdef DEBUG_INTC
@@ -289,7 +292,6 @@ static void sh_intc_write(void *opaque, target_phys_addr_t 
offset,
     }
 
     for (k = 0; k <= first; k++) {
-        int priority = -1;      
         unsigned shift = ((first - k) * width);
         mask = ((1 << width) - 1) << shift;
 
@@ -301,13 +303,14 @@ static void sh_intc_write(void *opaque, 
target_phys_addr_t offset,
 #endif
        if (mode & INTC_MODE_IS_PRIO) {
            assert (width == 4 || width == 8);
-           priority = (value & mask) >> shift;
+           int priority = (value & mask) >> shift;
            if (width == 8)
                priority &= 0x1f;
            else if (width == 4)
                priority <<= 1;
-       }
-       sh_intc_toggle_mask(desc, enum_ids[k], value & mask, priority, 0);
+           sh_intc_toggle_mask(desc, enum_ids[k], (priority>1), priority, 0);
+       } else
+           sh_intc_toggle_mask(desc, enum_ids[k], !(value & mask), -1, 0);
     }
 
     *valuep = value;
@@ -422,18 +425,18 @@ void sh_intc_register_sources(struct intc_desc *desc,
 
            /* Propagate group's enable_max to children.  */
            for (k = 0; k < ARRAY_SIZE(gr->enum_ids); k++) {
-               struct intc_source *child;
+               struct intc_source *child;
                 if (!gr->enum_ids[k])
                     continue;
 
                child = sh_intc_source(desc, gr->enum_ids[k]);
                child->enable_max += s->enable_max;
            }
-           
+
            /* Chain sources within each group via source->next_enum_id,
               so that we can easily enable/disable all sources in 
               a group later.  */
-           
+
            assert(s->next_enum_id == 0);
            s->next_enum_id = gr->enum_ids[0];
 
@@ -510,13 +513,13 @@ int sh_intc_init(struct intc_desc *desc,
     return 0;
 }
 
-/* Assert level <n> IRL interrupt. 
+/* Assert level <level> IRL interrupt. 
    0:deassert. 1:lowest priority,... 15:highest priority. */
 void sh_intc_set_irl(void *opaque, int n, int level)
 {
     struct intc_source *s = opaque;
     while ((s = sh_intc_source(s->parent, s->next_enum_id))) {
-       if (s->priority == level)
+       if (s->priority/2 == level)
            sh_intc_toggle_source(s, 0, s->asserted?0:1);
        else
            if (s->asserted)
@@ -531,6 +534,6 @@ void sh_intc_init_irl_priorities(struct intc_source *s)
 {
     int priority = 15;
     while ((s = sh_intc_source(s->parent, s->next_enum_id)))
-       s->priority = priority--;
+       s->priority = priority-- *2;
     assert(priority == 0);
 }
diff --git a/hw/sh_intc.h b/hw/sh_intc.h
index 5666497..83b646e 100644
--- a/hw/sh_intc.h
+++ b/hw/sh_intc.h
@@ -57,7 +57,7 @@ struct intc_desc {
     int iomemtype;
     int pending; /* number of interrupt sources that has pending set */
     target_phys_addr_t base;
-    unsigned icr0;    
+    unsigned icr0;
 };
 
 int sh_intc_get_pending_vector(struct intc_desc *desc, int *priority);
@@ -80,7 +80,7 @@ int sh_intc_init(struct intc_desc *desc,
                 int nr_prio_regs);
 
 void sh_intc_set_irl(void *opaque, int n, int level);
-                
+
 void sh_intc_init_irl_priorities(struct intc_source *s);
 
 #endif /* __SH_INTC_H__ */
-- 
1.5.6.3