[Qemu-devel] SH: Improve the interrupt controller

[Qemu-devel] SH: Improve the interrupt controller

[Vladimir Prus]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]


This patch improves the intc implementation in these ways:

   - On interrupt, the priority mask in SSR is updated,
     if OPM register tells it should be
   - We check interrupt priority and compare it with
     priority mask
   - Priorities for IRL interrupts (which are fixed), are
     assigned
   - The ICR register is supported, and LVLMODE bit, which
     controls if interrupt is automatically de-asserted,
     is implemented
   - A bug where handling of paired set mask / clear mask
     registers was done backward is fixed
   - A bug where enabling a group did not work was fixed.

I have tested this only with SH4A and it's desirable to test
with 7751/R2D. However, I no longer sure I know which kernel
to use for that. Can anybody either provide me with instructions,
or test this patch with R2D for me?

- Volodya

[-- Attachment #2: intc.diff --]
[-- Type: text/x-diff, Size: 14947 bytes --]

commit aac8877a31db83fe5cba30a742fc23b2dccee518
Author: Vladimir Prus <vladimir@codesourcery.com>
Date:   Sun Oct 12 20:02:08 2008 +0400

    SH: Improve the interrupt controller
    
    	* hw/sh7750.c (sh7750_init): Pass the base address to
    	sh_intc_init.
    	* hw/sh_intc.c (ICR0_LVLMODE): New.
    	(sh_intc_get_pending_vector): Remame parameter imask to priority,
    	and make it in/out.  Find source with the highest priority.
    	(sh_intc_toggle_mask): New parameter 'priority'. Set source
    	priority, if provided and pass priority to other members of a
    	group.
    	(sh_intc_read): Handle read of ICR.
    	(sh_intc_write): Handle write of ICR.  Fix masking in set/clear
    	register pair case.
    	(sh_intc_register_source, sh_intc_register_sources): If we have
    	priority register that controls a group, properly set enable_max
    	on the group members.
    	(sh_intc_init): Remember and register the base address.
    	(sh_intc_set_irl_priorities): New.
    	* hw/sh_intc.h (struct intc_source): New field priority.
    	(struct intc_desc): New fields base, icr0.
    	(sh_intc_get_pending_vector): Adjust prototype.
    	(sh_intc_init): New parameter 'base'.
    	(sh_intc_set_irl_priorities): New.
    	* target-sh4/cpu.h (struct CPUSH4State): New field opm.
    	* helper.c (do_interrupt): Update interrupt mask in SR if
    	necessary.

diff --git a/hw/sh7750.c b/hw/sh7750.c
index af86f0e..a43c2f1 100644
--- a/hw/sh7750.c
+++ b/hw/sh7750.c
@@ -701,6 +701,7 @@ SH7750State *sh7750_init(CPUSH4State * cpu)
 				 sh7750_mm_cache_and_tlb);
 
     sh_intc_init(&s->intc, NR_SOURCES,
+		 0x1fd00000,
 		 _INTC_ARRAY(mask_registers),
 		 _INTC_ARRAY(prio_registers));
 
diff --git a/hw/sh_intc.c b/hw/sh_intc.c
index 136e7dd..0f2ddf0 100644
--- a/hw/sh_intc.c
+++ b/hw/sh_intc.c
@@ -19,6 +19,8 @@
 #define INTC_A7(x) ((x) & 0x1fffffff)
 #define INTC_ARRAY(x) (sizeof(x) / sizeof(x[0]))
 
+#define ICR0_LVLMODE (1 << 21)
+
 void sh_intc_toggle_source(struct intc_source *source,
 			   int enable_adj, int assert_adj)
 {
@@ -46,8 +48,9 @@ void sh_intc_toggle_source(struct intc_source *source,
     if (pending_changed) {
         if (source->pending) {
             source->parent->pending++;
-	    if (source->parent->pending == 1)
+	    if (source->parent->pending == 1) {
                 cpu_interrupt(first_cpu, CPU_INTERRUPT_HARD);
+	    }
 	}
 	else {
             source->parent->pending--;
@@ -84,30 +87,46 @@ static void sh_intc_set_irq (void *opaque, int n, int level)
     sh_intc_toggle_source(source, 0, -1);
 }
 
-int sh_intc_get_pending_vector(struct intc_desc *desc, int imask)
+int sh_intc_get_pending_vector(struct intc_desc *desc, int *priority)
 {
     unsigned int i;
+    unsigned highest_priority = 0;
+    int found = -1;
 
-    /* slow: use a linked lists of pending sources instead */
-    /* wrong: take interrupt priority into account (one list per priority) */
+    /* slow: use a linked list of pending sources instead */
 
-    if (imask == 0x0f) {
-        return -1; /* FIXME, update code to include priority per source */
+    if (*priority == 0x0f) {
+        return -1;
     }
-
+    
     for (i = 0; i < desc->nr_sources; i++) {
         struct intc_source *source = desc->sources + i;
 
-	if (source->pending) {
+	if (source->pending && source->priority > highest_priority)
+	    {
+	        highest_priority = source->priority;
+		found = i;
+	    }
+    }
+
+    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",
+	printf("sh_intc: (%d) returning interrupt source 0x%x\n",
 		   desc->pending, source->vect);
 #endif
-            return source->vect;
-	}
-    }
 
-    assert(0);
+	if (desc->icr0 & ICR0_LVLMODE)
+	  sh_intc_toggle_source(source, 0, -1);
+
+	/* Priority in intc is 5 bits, whereas processor knows only 4 bits. */
+	*priority = highest_priority >> 1;
+	
+	return source->vect;
+    }
+    return -1;
 }
 
 #define INTC_MODE_NONE       0
@@ -187,7 +206,7 @@ static void sh_intc_locate(struct intc_desc *desc,
 }
 
 static void sh_intc_toggle_mask(struct intc_desc *desc, intc_enum id,
-				int enable, int is_group)
+				int enable, int priority, int is_group)
 {
     struct intc_source *source = desc->sources + id;
 
@@ -202,7 +221,11 @@ 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;
+      }
 
 #ifdef DEBUG_INTC
     else {
@@ -211,7 +234,7 @@ static void sh_intc_toggle_mask(struct intc_desc *desc, intc_enum id,
 #endif
 
     if ((is_group || !source->vect) && source->next_enum_id) {
-        sh_intc_toggle_mask(desc, source->next_enum_id, enable, 1);
+        sh_intc_toggle_mask(desc, source->next_enum_id, enable, priority, 1);
     }
 
 #ifdef DEBUG_INTC
@@ -233,6 +256,8 @@ static uint32_t sh_intc_read(void *opaque, target_phys_addr_t offset)
 #ifdef DEBUG_INTC
     printf("sh_intc_read 0x%lx\n", (unsigned long) offset);
 #endif
+    if (INTC_A7(offset) == INTC_A7(desc->base))
+      return desc->icr0;
 
     sh_intc_locate(desc, (unsigned long)offset, &valuep, 
 		   &enum_ids, &first, &width, &mode);
@@ -255,18 +280,26 @@ static void sh_intc_write(void *opaque, target_phys_addr_t offset,
     printf("sh_intc_write 0x%lx 0x%08x\n", (unsigned long) offset, value);
 #endif
 
+    if (INTC_A7(offset) == INTC_A7(desc->base))
+      {
+	desc->icr0 = value;
+	return;	
+      }
+
     sh_intc_locate(desc, (unsigned long)offset, &valuep, 
 		   &enum_ids, &first, &width, &mode);
 
     switch (mode) {
     case INTC_MODE_ENABLE_REG | INTC_MODE_IS_PRIO: break;
-    case INTC_MODE_DUAL_SET: value |= *valuep; break;
-    case INTC_MODE_DUAL_CLR: value = *valuep & ~value; break;
+    case INTC_MODE_DUAL_SET: value = *valuep & ~value; break;
+    case INTC_MODE_DUAL_CLR: value |= *valuep; break;
     default: assert(0);
     }
 
     for (k = 0; k <= first; k++) {
-        mask = ((1 << width) - 1) << ((first - k) * width);
+        int priority = -1;      
+        unsigned shift = ((first - k) * width);
+        mask = ((1 << width) - 1) << shift;
 
 	if ((*valuep & mask) == (value & mask))
             continue;
@@ -274,7 +307,18 @@ static void sh_intc_write(void *opaque, target_phys_addr_t offset,
 	printf("k = %d, first = %d, enum = %d, mask = 0x%08x\n", 
 	       k, first, enum_ids[k], (unsigned int)mask);
 #endif
-        sh_intc_toggle_mask(desc, enum_ids[k], value & mask, 0);
+	if (mode & INTC_MODE_IS_PRIO)
+	  {
+	    assert (width == 4 || width == 8);
+	    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);
+
     }
 
     *valuep = value;
@@ -316,24 +360,24 @@ static void sh_intc_register(struct intc_desc *desc,
 }
 
 static void sh_intc_register_source(struct intc_desc *desc,
-				    intc_enum source,
+				    intc_enum source_id,
 				    struct intc_group *groups,
 				    int nr_groups)
 {
     unsigned int i, k;
-    struct intc_source *s;
+    struct intc_source *source = sh_intc_source(desc, source_id);
+    assert(source);
 
     if (desc->mask_regs) {
         for (i = 0; i < desc->nr_mask_regs; i++) {
 	    struct intc_mask_reg *mr = desc->mask_regs + i;
 
 	    for (k = 0; k < INTC_ARRAY(mr->enum_ids); k++) {
-                if (mr->enum_ids[k] != source)
-                    continue;
-
-		s = sh_intc_source(desc, mr->enum_ids[k]);
-		if (s)
-                    s->enable_max++;
+	      if (mr->enum_ids[k] == source_id)
+		{
+		  source->enable_max++;
+		  break;
+		}
 	    }
 	}
     }
@@ -343,31 +387,14 @@ static void sh_intc_register_source(struct intc_desc *desc,
 	    struct intc_prio_reg *pr = desc->prio_regs + i;
 
 	    for (k = 0; k < INTC_ARRAY(pr->enum_ids); k++) {
-                if (pr->enum_ids[k] != source)
-                    continue;
-
-		s = sh_intc_source(desc, pr->enum_ids[k]);
-		if (s)
-                    s->enable_max++;
+	      if (pr->enum_ids[k] == source_id)
+		{
+		  source->enable_max++;
+		  break;
+		}
 	    }
 	}
     }
-
-    if (groups) {
-        for (i = 0; i < nr_groups; i++) {
-	    struct intc_group *gr = groups + i;
-
-	    for (k = 0; k < INTC_ARRAY(gr->enum_ids); k++) {
-                if (gr->enum_ids[k] != source)
-                    continue;
-
-		s = sh_intc_source(desc, gr->enum_ids[k]);
-		if (s)
-                    s->enable_max++;
-	    }
-	}
-    }
-
 }
 
 void sh_intc_register_sources(struct intc_desc *desc,
@@ -377,7 +404,7 @@ void sh_intc_register_sources(struct intc_desc *desc,
 			      int nr_groups)
 {
     unsigned int i, k;
-    struct intc_source *s;
+    struct intc_source *s, *s2;
 
     for (i = 0; i < nr_vectors; i++) {
 	struct intc_vect *vect = vectors + i;
@@ -394,10 +421,34 @@ void sh_intc_register_sources(struct intc_desc *desc,
     }
 
     if (groups) {
+      /* First of all, register group's sources, so that enable_max is property
+	 set.  */
+      for (i = 0; i < nr_groups; i++) {
+	struct intc_group *gr = groups + i;
+
+	sh_intc_register_source(desc, gr->enum_id, groups, nr_groups);
+      }
+
+
         for (i = 0; i < nr_groups; i++) {
 	    struct intc_group *gr = groups + i;
 
 	    s = sh_intc_source(desc, gr->enum_id);
+
+	    /* Propagate group's enable_max to children.  */
+	    for (k = 0; k < INTC_ARRAY(gr->enum_ids); k++) {
+                if (!gr->enum_ids[k])
+                    continue;
+
+		s2 = sh_intc_source(desc, gr->enum_ids[k]);
+		s2->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];
 
 	    for (k = 1; k < INTC_ARRAY(gr->enum_ids); k++) {
@@ -405,6 +456,7 @@ void sh_intc_register_sources(struct intc_desc *desc,
                     continue;
 
 		s = sh_intc_source(desc, gr->enum_ids[k - 1]);
+		assert(s->next_enum_id == 0);
 		s->next_enum_id = gr->enum_ids[k];
 	    }
 
@@ -417,6 +469,7 @@ void sh_intc_register_sources(struct intc_desc *desc,
 }
 
 int sh_intc_init(struct intc_desc *desc,
+		 target_phys_addr_t base,
 		 int nr_sources,
 		 struct intc_mask_reg *mask_regs,
 		 int nr_mask_regs,
@@ -431,6 +484,7 @@ int sh_intc_init(struct intc_desc *desc,
     desc->nr_mask_regs = nr_mask_regs;
     desc->prio_regs = prio_regs;
     desc->nr_prio_regs = nr_prio_regs;
+    desc->base = base;
 
     i = sizeof(struct intc_source) * nr_sources;
     desc->sources = malloc(i);
@@ -448,6 +502,10 @@ int sh_intc_init(struct intc_desc *desc,
  
     desc->iomemtype = cpu_register_io_memory(0, sh_intc_readfn,
 					     sh_intc_writefn, desc);
+
+    cpu_register_physical_memory_offset(A7ADDR(base), 4, desc->iomemtype, INTC_A7(base));
+    cpu_register_physical_memory_offset(P4ADDR(base), 4, desc->iomemtype, INTC_A7(base));
+
     if (desc->mask_regs) {
         for (i = 0; i < desc->nr_mask_regs; i++) {
 	    struct intc_mask_reg *mr = desc->mask_regs + i;
@@ -483,3 +541,19 @@ void sh_intc_set_irl(void *opaque, int n, int level)
 	        sh_intc_toggle_source(s, 0, -1);
     }
 }
+
+void sh_intc_set_irl_priorities(struct intc_desc *desc,
+				int low_priority_source, 
+				int high_priority_source)
+{
+  int priority = 15;
+  int i;
+
+  assert (low_priority_source - high_priority_source + 1 == 15);
+  for (i = high_priority_source; i <= low_priority_source; ++i, --priority)
+    {
+      struct intc_source *sources = desc->sources + i;
+      sources->priority = priority;
+    }
+  assert(priority == 0);
+}
diff --git a/hw/sh_intc.h b/hw/sh_intc.h
index 4e36f00..d35f8dc 100644
--- a/hw/sh_intc.h
+++ b/hw/sh_intc.h
@@ -42,6 +42,7 @@ struct intc_source {
     int enable_count;
     int enable_max;
     int pending; /* emulates the result of signal and masking */
+    int priority;
     struct intc_desc *parent;
 };
 
@@ -55,9 +56,11 @@ struct intc_desc {
     int nr_prio_regs;
     int iomemtype;
     int pending; /* number of interrupt sources that has pending set */
+    target_phys_addr_t base;
+    unsigned icr0;    
 };
 
-int sh_intc_get_pending_vector(struct intc_desc *desc, int imask);
+int sh_intc_get_pending_vector(struct intc_desc *desc, int *priority);
 struct intc_source *sh_intc_source(struct intc_desc *desc, intc_enum id);
 void sh_intc_toggle_source(struct intc_source *source,
 			   int enable_adj, int assert_adj);
@@ -69,6 +72,7 @@ void sh_intc_register_sources(struct intc_desc *desc,
 			      int nr_groups);
 
 int sh_intc_init(struct intc_desc *desc,
+		 target_phys_addr_t base,
 		 int nr_sources,
 		 struct intc_mask_reg *mask_regs,
 		 int nr_mask_regs,
@@ -76,5 +80,14 @@ int sh_intc_init(struct intc_desc *desc,
 		 int nr_prio_regs);
 
 void sh_intc_set_irl(void *opaque, int n, int level);
+		 
+/* Assign priorities for IRL interrupt sources. The
+   low_priority_source gets priority of 1, and
+   high_priority_source gets priority of 15.  Sources in
+   between get priorities in between. The number of
+   sources in the range should be 15.  */
+void sh_intc_set_irl_priorities(struct intc_desc *desc,
+				int low_priority_source, 
+				int high_priority_source);
 
 #endif /* __SH_INTC_H__ */
diff --git a/target-sh4/cpu.h b/target-sh4/cpu.h
index eed3b1b..fe02a24 100644
--- a/target-sh4/cpu.h
+++ b/target-sh4/cpu.h
@@ -140,6 +140,7 @@ typedef struct CPUSH4State {
     uint32_t pvr;		/* Processor Version Register */
     uint32_t prr;		/* Processor Revision Register */
     uint32_t cvr;		/* Cache Version Register */
+    uint32_t opm;               /* CPU Operation Mode Register */
 
     uint32_t ldst;
 
diff --git a/target-sh4/helper.c b/target-sh4/helper.c
index 61b668b..10bcf84 100644
--- a/target-sh4/helper.c
+++ b/target-sh4/helper.c
@@ -81,6 +81,7 @@ void do_interrupt(CPUState * env)
 {
     int do_irq = env->interrupt_request & CPU_INTERRUPT_HARD;
     int do_exp, irq_vector = env->exception_index;
+    int priority = (env->sr >> 4) & 0xf;
 
     /* prioritize exceptions over interrupts */
 
@@ -99,7 +100,7 @@ void do_interrupt(CPUState * env)
 
     if (do_irq) {
         irq_vector = sh_intc_get_pending_vector(env->intc_handle,
-						(env->sr >> 4) & 0xf);
+						&priority);
         if (irq_vector == -1) {
             return; /* masked */
 	}
@@ -157,6 +158,14 @@ void do_interrupt(CPUState * env)
     }
 
     env->ssr = env->sr;
+
+    if (env->opm & (1 << 3))
+      {
+	unsigned mask = 0xf << 4;
+	env->ssr &= ~mask;
+	env->ssr |= (priority << 4) & mask;
+      }
+
     env->spc = env->pc;
     env->sgr = env->gregs[15];
     env->sr |= SR_BL | SR_MD | SR_RB;

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 22:52 Thu 11 Dec     , Vladimir Prus wrote:
> 
> This patch improves the intc implementation in these ways:
> 
>    - On interrupt, the priority mask in SSR is updated,
>      if OPM register tells it should be
>    - We check interrupt priority and compare it with
>      priority mask
>    - Priorities for IRL interrupts (which are fixed), are
>      assigned
>    - The ICR register is supported, and LVLMODE bit, which
>      controls if interrupt is automatically de-asserted,
>      is implemented
>    - A bug where handling of paired set mask / clear mask
>      registers was done backward is fixed
>    - A bug where enabling a group did not work was fixed.
I'll be better to split this patch in two on for the bug fix and one
for the improvements

IMHO your patch need some coding style cleanup also

Best Regards,
J.

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

[Vladimir Prus]

Jean-Christophe PLAGNIOL-VILLARD wrote:

> On 22:52 Thu 11 Dec     , Vladimir Prus wrote:
>> 
>> This patch improves the intc implementation in these ways:
>> 
>>    - On interrupt, the priority mask in SSR is updated,
>>      if OPM register tells it should be
>>    - We check interrupt priority and compare it with
>>      priority mask
>>    - Priorities for IRL interrupts (which are fixed), are
>>      assigned
>>    - The ICR register is supported, and LVLMODE bit, which
>>      controls if interrupt is automatically de-asserted,
>>      is implemented
>>    - A bug where handling of paired set mask / clear mask
>>      registers was done backward is fixed
>>    - A bug where enabling a group did not work was fixed.
> I'll be better to split this patch in two on for the bug fix and one
> for the improvements

FWIW, the most intrusive change is the bugfix for groups, so such
split won't make the patch more reviewable.

> IMHO your patch need some coding style cleanup also

Can you be more specific?

- Volodya

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

[takasi-y]

# a patch attached is not to be commit to repos, but for explanation.
Hi,

> I have tested this only with SH4A and it's desirable to test
> with 7751/R2D. However, I no longer sure I know which kernel
> to use for that. Can anybody either provide me with instructions,
> or test this patch with R2D for me?

It doesn't work at least here for me (for r2d).
It stops with CPU load 100% (and I had to power my PC off...).

BTW, I think your patch is not following to current sh_intc's design model.
# please read to the end. this will be turned over later.

I think its design model is
 - registering memory regions for each registers.
 - read/write function doesn't check address
But, you have add address check in read/write function.

Mine(which I have forgotten to send..., only to move icr) is attached at the
 end of this mail. This one is based on the recognition above.

It works, but I wonder if this model is valid or not on mmio system after #5849.
Especially here is in question,
+    cpu_register_physical_memory(P4ADDR(base), 2, io_memory);
+    cpu_register_physical_memory(A7ADDR(base), 2, io_memory);

Paul(the author of #5849) said,
> [1] It's actually the offset from the start of the first page of that region. 
> In practice this difference doesn't matter, and makes the implementation a 
> lot simpler.

I don't know why he thought it "doesn't matter", but from discussions on ML,
 I guess the model he have is
 - register one memory region for each modules.
 - read/write function check address
 - most of modules are aligned to page.

In that case, your patch is nearer to the model.
But, then, we should done with these,
        cpu_register_physical_memory_offset(P4ADDR(address), 4,
                                            desc->iomemtype, INTC_A7(address));
        cpu_register_physical_memory_offset(A7ADDR(address), 4,
                                            desc->iomemtype, INTC_A7(address));

Sorry for no conclusion. I'm still wondering.
/yoshii

commit 53bf0820c255aea216053e67c11ffd409b78c16d
Author: Takashi YOSHII <takasi-y@ops.dti.ne.jp>
Date:   Thu Dec 11 22:26:19 2008 +0900

    move intc.icr from sh7750.c to sh_intc.c

diff --git a/hw/sh7750.c b/hw/sh7750.c
index 4d1a806..0c49c4c 100644
--- a/hw/sh7750.c
+++ b/hw/sh7750.c
@@ -700,7 +700,7 @@ SH7750State *sh7750_init(CPUSH4State * cpu)
     cpu_register_physical_memory(0xf0000000, 0x08000000,
 				 sh7750_mm_cache_and_tlb);
 
-    sh_intc_init(&s->intc, NR_SOURCES,
+    sh_intc_init(0x1fd00000, &s->intc, NR_SOURCES,
 		 _INTC_ARRAY(mask_registers),
 		 _INTC_ARRAY(prio_registers));
 
diff --git a/hw/sh_intc.c b/hw/sh_intc.c
index 136e7dd..7c66545 100644
--- a/hw/sh_intc.c
+++ b/hw/sh_intc.c
@@ -416,7 +416,26 @@ void sh_intc_register_sources(struct intc_desc *desc,
     }
 }
 
-int sh_intc_init(struct intc_desc *desc,
+static uint32_t sh_intc_icr_read(void *opaque, uint32_t offs)
+{
+    return ((struct intc_desc*)opaque)->icr;
+}
+
+static void sh_intc_icr_write(void *opaque, uint32_t offs, uint32_t value)
+{
+    ((struct intc_desc*)opaque)->icr = value;
+}
+
+static CPUReadMemoryFunc *sh_intc_icr_readfn[] = {
+    NULL, sh_intc_icr_read, NULL
+};
+
+static CPUWriteMemoryFunc *sh_intc_icr_writefn[] = {
+    NULL, sh_intc_icr_write, NULL
+};
+
+int sh_intc_init(target_phys_addr_t base,
+		 struct intc_desc *desc,
 		 int nr_sources,
 		 struct intc_mask_reg *mask_regs,
 		 int nr_mask_regs,
@@ -424,6 +443,7 @@ int sh_intc_init(struct intc_desc *desc,
 		 int nr_prio_regs)
 {
     unsigned int i;
+    int io_memory;
 
     desc->pending = 0;
     desc->nr_sources = nr_sources;
@@ -465,6 +485,10 @@ int sh_intc_init(struct intc_desc *desc,
 	    sh_intc_register(desc, pr->clr_reg);
 	}
     }
+    io_memory = cpu_register_io_memory(0, sh_intc_icr_readfn,
+                                       sh_intc_icr_writefn, desc);
+    cpu_register_physical_memory(P4ADDR(base), 2, io_memory);
+    cpu_register_physical_memory(A7ADDR(base), 2, io_memory);
 
     return 0;
 }
diff --git a/hw/sh_intc.h b/hw/sh_intc.h
index 4e36f00..e39f794 100644
--- a/hw/sh_intc.h
+++ b/hw/sh_intc.h
@@ -55,6 +55,7 @@ struct intc_desc {
     int nr_prio_regs;
     int iomemtype;
     int pending; /* number of interrupt sources that has pending set */
+    uint16_t icr;
 };
 
 int sh_intc_get_pending_vector(struct intc_desc *desc, int imask);
@@ -68,7 +69,8 @@ void sh_intc_register_sources(struct intc_desc *desc,
 			      struct intc_group *groups,
 			      int nr_groups);
 
-int sh_intc_init(struct intc_desc *desc,
+int sh_intc_init(target_phys_addr_t base,
+		 struct intc_desc *desc,
 		 int nr_sources,
 		 struct intc_mask_reg *mask_regs,
 		 int nr_mask_regs,

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

[Vladimir Prus]

On Sunday 14 December 2008 19:46:39 takasi-y@ops.dti.ne.jp wrote:
> # a patch attached is not to be commit to repos, but for explanation.
> Hi,
> 
> > I have tested this only with SH4A and it's desirable to test
> > with 7751/R2D. However, I no longer sure I know which kernel
> > to use for that. Can anybody either provide me with instructions,
> > or test this patch with R2D for me?
> 
> It doesn't work at least here for me (for r2d).
> It stops with CPU load 100% (and I had to power my PC off...).

Bummer. Can you send me the kernel/rootfs combo you are using (either
link, or complete binary offlist) and I'll see what I've broken.

> BTW, I think your patch is not following to current sh_intc's design model.
> # please read to the end. this will be turned over later.
> 
> I think its design model is
>  - registering memory regions for each registers.
>  - read/write function doesn't check address
> But, you have add address check in read/write function.

It seems to me that read/write function does check address. Consider:

	static uint32_t sh_intc_read(void *opaque, target_phys_addr_t offset)
	{
	    ...
	    sh_intc_locate(desc, (unsigned long)offset, &valuep, 
			   &enum_ids, &first, &width, &mode);
            ...
	}

	static void sh_intc_locate(struct intc_desc *desc,
			   unsigned long address,
			   unsigned long **datap,
			   intc_enum **enums,
			   unsigned int *first,
			   unsigned int *width,
			   unsigned int *modep)
	{
	    ....
	    if (desc->mask_regs) {
                ...
	    	mode = sh_intc_mode(address, mr->set_reg, mr->clr_reg);
	        if (mode == INTC_MODE_NONE)
                    continue;
                ...
            }
            ...
        }
    }

So, read/write function do check addresses. On the other hand, each register
is registered separately. 

> 
> Mine(which I have forgotten to send..., only to move icr) is attached at the
>  end of this mail. This one is based on the recognition above.
> 
> It works, but I wonder if this model is valid or not on mmio system after #5849.
> Especially here is in question,
> +    cpu_register_physical_memory(P4ADDR(base), 2, io_memory);
> +    cpu_register_physical_memory(A7ADDR(base), 2, io_memory);
> 
> Paul(the author of #5849) said,
> > [1] It's actually the offset from the start of the first page of that region. 
> > In practice this difference doesn't matter, and makes the implementation a 
> > lot simpler.
> 
> I don't know why he thought it "doesn't matter", but from discussions on ML,
>  I guess the model he have is
>  - register one memory region for each modules.
>  - read/write function check address
>  - most of modules are aligned to page.

It is my understanding too. It seems that it's not hard to move sh_intc to this
model -- we just need to register entire region, as opposed to registering
memory for each register.

> In that case, your patch is nearer to the model.
> But, then, we should done with these,
>         cpu_register_physical_memory_offset(P4ADDR(address), 4,
>                                             desc->iomemtype, INTC_A7(address));
>         cpu_register_physical_memory_offset(A7ADDR(address), 4,
>                                             desc->iomemtype, INTC_A7(address));

Yes, both these, and sh_intc_register should not do piecewise registration if
we decide to change model.

- Volodya

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

[Vladimir Prus]

On Sunday 14 December 2008 19:57:36 Vladimir Prus wrote:
> On Sunday 14 December 2008 19:46:39 takasi-y@ops.dti.ne.jp wrote:
> > # a patch attached is not to be commit to repos, but for explanation.
> > Hi,
> > 
> > > I have tested this only with SH4A and it's desirable to test
> > > with 7751/R2D. However, I no longer sure I know which kernel
> > > to use for that. Can anybody either provide me with instructions,
> > > or test this patch with R2D for me?
> > 
> > It doesn't work at least here for me (for r2d).
> > It stops with CPU load 100% (and I had to power my PC off...).
> 
> Bummer. Can you send me the kernel/rootfs combo you are using (either
> link, or complete binary offlist) and I'll see what I've broken.

Yoshii,

I wonder if you've missed the above email. I would like to get these
patches in, but for that I need to make sure they don't break r2d, and
for that, I need to test with r2d kernel/rootfs -- preferrably the
same that you use for testing.

Thanks,
Volodya

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

[Shin-ichiro KAWASAKI]

Vladimir Prus wrote:
> On Sunday 14 December 2008 19:57:36 Vladimir Prus wrote:
>> On Sunday 14 December 2008 19:46:39 takasi-y@ops.dti.ne.jp wrote:
>>> # a patch attached is not to be commit to repos, but for explanation.
>>> Hi,
>>>
>>>> I have tested this only with SH4A and it's desirable to test
>>>> with 7751/R2D. However, I no longer sure I know which kernel
>>>> to use for that. Can anybody either provide me with instructions,
>>>> or test this patch with R2D for me?
>>> It doesn't work at least here for me (for r2d).
>>> It stops with CPU load 100% (and I had to power my PC off...).
>> Bummer. Can you send me the kernel/rootfs combo you are using (either
>> link, or complete binary offlist) and I'll see what I've broken.
> 
> Yoshii,
> 
> I wonder if you've missed the above email. I would like to get these
> patches in, but for that I need to make sure they don't break r2d, and
> for that, I need to test with r2d kernel/rootfs -- preferrably the
> same that you use for testing.

I tried to apply your patch to rev 5983, and found that it causes
segmentation fault at start up when I tried to run R2D+ board emulation.
Could you try the following set?

Kernel
http://www.assembla.com/spaces/qemu-sh4/documents/b1ftw8fRyr3Bfnab7jnrAJ/download?filename=Kernel%2Bto%2Btest%2BSH4%2BSystem%2BEmulation

Userland (Thx to Kristoffer Ericson)
http://jlime.com/downloads/development/qemu/disk_small.img

Command line example
% ./qemu/sh4-softmmu/qemu-system-sh4 -M r2d -serial vc -serial stdio -m 1024M -kernel ./linux-2.6/arch/sh/boot/zImage -hda disk_small.img


Regards,
Shin-ichiro KAWASAKI

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

[takasi-y]

Hi,

me> It stops with CPU load 100% (and I had to power my PC off...).
The reason was because of a mismatch between
> diff --git a/hw/sh7750.c b/hw/sh7750.c
...
>      sh_intc_init(&s->intc, NR_SOURCES,
> +		 0x1fd00000,
>  		 _INTC_ARRAY(mask_registers),
and 
>  int sh_intc_init(struct intc_desc *desc,
> +		 target_phys_addr_t base,
>  		 int nr_sources,
.
This uses up CPU and memory.
Please forget about this issue, because you must have the correct code.

Even with this fixed, I can't get r2d working here. But, never mind.
Apparently, we are not sharing the same code.
I'd like you to re-post the patch from your current code, with a pointer
to base code (svn rev#, git hash or so).
# BTW, Unfortunately I don't have "proven" kernel binary, either.
/yoshii

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

[takasi-y]

Hi, Vladimir.

I managed to make the code working.
r2d boot, SCI and CF working, and /proc/interrupts increases.

Essential points modified are below.
1. sh_intc_init() caller/callee mismatch.
2. nobody calls sh_intc_set_irl_priorities()
3. INTC_MODE_DUAL_SET/CLR swapped
I'm not sure 3 in your patch is on purpose or not.

Attached patch is a diff against rev#6563.
This is yours + my small fixes, which are..
- Above three
- sh_intc_set_irl_priorities() has switched to sh_intc_init_irl_priorities().
- sh_intc_set_irl()'s enable hack removed.
- indent,tab/space,brace changed (to what looks like code around)
- reduce INTC_A7() usage
- some others.

/yoshii
---
 hw/sh7750.c         |    9 ++-
 hw/sh_intc.c        |  156 ++++++++++++++++++++++++++++++++++-----------------
 hw/sh_intc.h        |    8 ++-
 target-sh4/cpu.h    |    1 +
 target-sh4/helper.c |   10 +++-
 5 files changed, 127 insertions(+), 57 deletions(-)

diff --git a/hw/sh7750.c b/hw/sh7750.c
index 423c43f..57a5d7d 100644
--- a/hw/sh7750.c
+++ b/hw/sh7750.c
@@ -732,7 +732,7 @@ SH7750State *sh7750_init(CPUSH4State * cpu)
     cpu_register_physical_memory(0xf0000000, 0x08000000,
 				 sh7750_mm_cache_and_tlb);
 
-    sh_intc_init(&s->intc, NR_SOURCES,
+    sh_intc_init(&s->intc, 0x1fd00000, NR_SOURCES,
 		 _INTC_ARRAY(mask_registers),
 		 _INTC_ARRAY(prio_registers));
 
@@ -806,7 +806,8 @@ SH7750State *sh7750_init(CPUSH4State * cpu)
 
 qemu_irq sh7750_irl(SH7750State *s)
 {
-    sh_intc_toggle_source(sh_intc_source(&s->intc, IRL), 1, 0); /* enable */
-    return qemu_allocate_irqs(sh_intc_set_irl, sh_intc_source(&s->intc, IRL),
-                               1)[0];
+    struct intc_source *irl = sh_intc_source(&s->intc, IRL);
+
+    sh_intc_init_irl_priorities(irl);
+    return qemu_allocate_irqs(sh_intc_set_irl, irl, 1)[0];
 }
diff --git a/hw/sh_intc.c b/hw/sh_intc.c
index f4138fd..7641b11 100644
--- a/hw/sh_intc.c
+++ b/hw/sh_intc.c
@@ -18,6 +18,8 @@
 
 #define INTC_A7(x) ((x) & 0x1fffffff)
 
+#define ICR0_LVLMODE (1 << 21)
+
 void sh_intc_toggle_source(struct intc_source *source,
 			   int enable_adj, int assert_adj)
 {
@@ -83,30 +85,42 @@ static void sh_intc_set_irq (void *opaque, int n, int level)
     sh_intc_toggle_source(source, 0, -1);
 }
 
-int sh_intc_get_pending_vector(struct intc_desc *desc, int imask)
+int sh_intc_get_pending_vector(struct intc_desc *desc, int *priority)
 {
     unsigned int i;
+    unsigned highest_priority = 0;
+    int found = -1;
 
-    /* slow: use a linked lists of pending sources instead */
-    /* wrong: take interrupt priority into account (one list per priority) */
+    /* slow: use a linked list of pending sources instead */
 
-    if (imask == 0x0f) {
-        return -1; /* FIXME, update code to include priority per source */
-    }
+    if (*priority == 0x0f)
+        return -1;
 
     for (i = 0; i < desc->nr_sources; i++) {
         struct intc_source *source = desc->sources + i;
 
-	if (source->pending) {
-#ifdef DEBUG_INTC_SOURCES
-            printf("sh_intc: (%d) returning interrupt source 0x%x\n",
-		   desc->pending, source->vect);
-#endif
-            return source->vect;
+	if (source->pending && source->priority > highest_priority) {
+	    highest_priority = source->priority;
+	    found = i;
 	}
     }
 
-    assert(0);
+    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);
+#endif
+	if (desc->icr0 & ICR0_LVLMODE)
+	    sh_intc_toggle_source(source, 0, -1);
+
+	/* Priority in intc is 5 bits, whereas processor knows only 4 bits. */
+	*priority = highest_priority >> 1;
+	
+	return source->vect;
+    }
+    return -1;
 }
 
 #define INTC_MODE_NONE       0
@@ -186,7 +200,7 @@ static void sh_intc_locate(struct intc_desc *desc,
 }
 
 static void sh_intc_toggle_mask(struct intc_desc *desc, intc_enum id,
-				int enable, int is_group)
+				int enable, int priority, int is_group)
 {
     struct intc_source *source = desc->sources + id;
 
@@ -200,8 +214,11 @@ static void sh_intc_toggle_mask(struct intc_desc *desc, intc_enum id,
 	return;
     }
 
-    if (source->vect)
+    if (source->vect) {
         sh_intc_toggle_source(source, enable ? 1 : -1, 0);
+        if (priority != -1)
+            source->priority = priority;
+    }
 
 #ifdef DEBUG_INTC
     else {
@@ -210,7 +227,7 @@ static void sh_intc_toggle_mask(struct intc_desc *desc, intc_enum id,
 #endif
 
     if ((is_group || !source->vect) && source->next_enum_id) {
-        sh_intc_toggle_mask(desc, source->next_enum_id, enable, 1);
+        sh_intc_toggle_mask(desc, source->next_enum_id, enable, priority, 1);
     }
 
 #ifdef DEBUG_INTC
@@ -232,6 +249,8 @@ static uint32_t sh_intc_read(void *opaque, target_phys_addr_t offset)
 #ifdef DEBUG_INTC
     printf("sh_intc_read 0x%lx\n", (unsigned long) offset);
 #endif
+    if (offset == desc->base)
+	return desc->icr0;
 
     sh_intc_locate(desc, (unsigned long)offset, &valuep, 
 		   &enum_ids, &first, &width, &mode);
@@ -254,6 +273,11 @@ static void sh_intc_write(void *opaque, target_phys_addr_t offset,
     printf("sh_intc_write 0x%lx 0x%08x\n", (unsigned long) offset, value);
 #endif
 
+    if (offset == desc->base) {
+	desc->icr0 = value;
+	return;	
+    }
+
     sh_intc_locate(desc, (unsigned long)offset, &valuep, 
 		   &enum_ids, &first, &width, &mode);
 
@@ -265,7 +289,9 @@ static void sh_intc_write(void *opaque, target_phys_addr_t offset,
     }
 
     for (k = 0; k <= first; k++) {
-        mask = ((1 << width) - 1) << ((first - k) * width);
+        int priority = -1;      
+        unsigned shift = ((first - k) * width);
+        mask = ((1 << width) - 1) << shift;
 
 	if ((*valuep & mask) == (value & mask))
             continue;
@@ -273,7 +299,15 @@ static void sh_intc_write(void *opaque, target_phys_addr_t offset,
 	printf("k = %d, first = %d, enum = %d, mask = 0x%08x\n", 
 	       k, first, enum_ids[k], (unsigned int)mask);
 #endif
-        sh_intc_toggle_mask(desc, enum_ids[k], value & mask, 0);
+	if (mode & INTC_MODE_IS_PRIO) {
+	    assert (width == 4 || width == 8);
+	    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);
     }
 
     *valuep = value;
@@ -320,19 +354,18 @@ static void sh_intc_register_source(struct intc_desc *desc,
 				    int nr_groups)
 {
     unsigned int i, k;
-    struct intc_source *s;
+    struct intc_source *s = sh_intc_source(desc, source);
+    assert(s);
 
     if (desc->mask_regs) {
         for (i = 0; i < desc->nr_mask_regs; i++) {
 	    struct intc_mask_reg *mr = desc->mask_regs + i;
 
 	    for (k = 0; k < ARRAY_SIZE(mr->enum_ids); k++) {
-                if (mr->enum_ids[k] != source)
-                    continue;
-
-		s = sh_intc_source(desc, mr->enum_ids[k]);
-		if (s)
-                    s->enable_max++;
+		if (mr->enum_ids[k] == source) {
+		    s->enable_max++;
+		    break;
+		}
 	    }
 	}
     }
@@ -342,31 +375,13 @@ static void sh_intc_register_source(struct intc_desc *desc,
 	    struct intc_prio_reg *pr = desc->prio_regs + i;
 
 	    for (k = 0; k < ARRAY_SIZE(pr->enum_ids); k++) {
-                if (pr->enum_ids[k] != source)
-                    continue;
-
-		s = sh_intc_source(desc, pr->enum_ids[k]);
-		if (s)
-                    s->enable_max++;
-	    }
-	}
-    }
-
-    if (groups) {
-        for (i = 0; i < nr_groups; i++) {
-	    struct intc_group *gr = groups + i;
-
-	    for (k = 0; k < ARRAY_SIZE(gr->enum_ids); k++) {
-                if (gr->enum_ids[k] != source)
-                    continue;
-
-		s = sh_intc_source(desc, gr->enum_ids[k]);
-		if (s)
-                    s->enable_max++;
+		if (pr->enum_ids[k] == source) {
+		    s->enable_max++;
+		    break;
+		}
 	    }
 	}
     }
-
 }
 
 void sh_intc_register_sources(struct intc_desc *desc,
@@ -393,10 +408,33 @@ void sh_intc_register_sources(struct intc_desc *desc,
     }
 
     if (groups) {
+	/* First of all, register group's sources, so that enable_max is
+	   property set.  */
+	for (i = 0; i < nr_groups; i++) {
+	    struct intc_group *gr = groups + i;
+	    sh_intc_register_source(desc, gr->enum_id, groups, nr_groups);
+	}
+
         for (i = 0; i < nr_groups; i++) {
 	    struct intc_group *gr = groups + i;
 
 	    s = sh_intc_source(desc, gr->enum_id);
+
+	    /* Propagate group's enable_max to children.  */
+	    for (k = 0; k < ARRAY_SIZE(gr->enum_ids); k++) {
+    		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];
 
 	    for (k = 1; k < ARRAY_SIZE(gr->enum_ids); k++) {
@@ -404,6 +442,7 @@ void sh_intc_register_sources(struct intc_desc *desc,
                     continue;
 
 		s = sh_intc_source(desc, gr->enum_ids[k - 1]);
+		assert(s->next_enum_id == 0);
 		s->next_enum_id = gr->enum_ids[k];
 	    }
 
@@ -416,6 +455,7 @@ void sh_intc_register_sources(struct intc_desc *desc,
 }
 
 int sh_intc_init(struct intc_desc *desc,
+		 target_phys_addr_t base,
 		 int nr_sources,
 		 struct intc_mask_reg *mask_regs,
 		 int nr_mask_regs,
@@ -430,6 +470,7 @@ int sh_intc_init(struct intc_desc *desc,
     desc->nr_mask_regs = nr_mask_regs;
     desc->prio_regs = prio_regs;
     desc->nr_prio_regs = nr_prio_regs;
+    desc->base = INTC_A7(base);
 
     i = sizeof(struct intc_source) * nr_sources;
     desc->sources = qemu_malloc(i);
@@ -445,6 +486,9 @@ int sh_intc_init(struct intc_desc *desc,
  
     desc->iomemtype = cpu_register_io_memory(0, sh_intc_readfn,
 					     sh_intc_writefn, desc);
+
+    sh_intc_register(desc, base); /* icr0 */
+
     if (desc->mask_regs) {
         for (i = 0; i < desc->nr_mask_regs; i++) {
 	    struct intc_mask_reg *mr = desc->mask_regs + i;
@@ -471,12 +515,22 @@ int sh_intc_init(struct intc_desc *desc,
 void sh_intc_set_irl(void *opaque, int n, int level)
 {
     struct intc_source *s = opaque;
-    int i, irl = level ^ 15;
-    for (i = 0; (s = sh_intc_source(s->parent, s->next_enum_id)); i++) {
-	if (i == irl)
-	    sh_intc_toggle_source(s, s->enable_count?0:1, s->asserted?0:1);
+    while ((s = sh_intc_source(s->parent, s->next_enum_id))) {
+	if (s->priority == level)
+	    sh_intc_toggle_source(s, 0, s->asserted?0:1);
 	else
 	    if (s->asserted)
 	        sh_intc_toggle_source(s, 0, -1);
     }
 }
+
+/* Initialize priorities for IRL interrupt sources.
+   Member #1 of the IRL group is set to 15, ... #15 is set to 1.
+   The number of members must be 15 */
+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--;
+    assert(priority == 0);
+}
diff --git a/hw/sh_intc.h b/hw/sh_intc.h
index a9750ae..5666497 100644
--- a/hw/sh_intc.h
+++ b/hw/sh_intc.h
@@ -42,6 +42,7 @@ struct intc_source {
     int enable_count;
     int enable_max;
     int pending; /* emulates the result of signal and masking */
+    int priority;
     struct intc_desc *parent;
 };
 
@@ -55,9 +56,11 @@ struct intc_desc {
     int nr_prio_regs;
     int iomemtype;
     int pending; /* number of interrupt sources that has pending set */
+    target_phys_addr_t base;
+    unsigned icr0;    
 };
 
-int sh_intc_get_pending_vector(struct intc_desc *desc, int imask);
+int sh_intc_get_pending_vector(struct intc_desc *desc, int *priority);
 struct intc_source *sh_intc_source(struct intc_desc *desc, intc_enum id);
 void sh_intc_toggle_source(struct intc_source *source,
 			   int enable_adj, int assert_adj);
@@ -69,6 +72,7 @@ void sh_intc_register_sources(struct intc_desc *desc,
 			      int nr_groups);
 
 int sh_intc_init(struct intc_desc *desc,
+		 target_phys_addr_t base,
 		 int nr_sources,
 		 struct intc_mask_reg *mask_regs,
 		 int nr_mask_regs,
@@ -76,5 +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__ */
diff --git a/target-sh4/cpu.h b/target-sh4/cpu.h
index 86a4a6b..0051bcd 100644
--- a/target-sh4/cpu.h
+++ b/target-sh4/cpu.h
@@ -139,6 +139,7 @@ typedef struct CPUSH4State {
     uint32_t pvr;		/* Processor Version Register */
     uint32_t prr;		/* Processor Revision Register */
     uint32_t cvr;		/* Cache Version Register */
+    uint32_t opm;		/* CPU Operation Mode Register */
 
      CPU_COMMON tlb_t utlb[UTLB_SIZE];	/* unified translation table */
     tlb_t itlb[ITLB_SIZE];	/* instruction translation table */
diff --git a/target-sh4/helper.c b/target-sh4/helper.c
index 7f5430a..cf52cbf 100644
--- a/target-sh4/helper.c
+++ b/target-sh4/helper.c
@@ -81,6 +81,7 @@ void do_interrupt(CPUState * env)
 {
     int do_irq = env->interrupt_request & CPU_INTERRUPT_HARD;
     int do_exp, irq_vector = env->exception_index;
+    int priority = (env->sr >> 4) & 0xf;
 
     /* prioritize exceptions over interrupts */
 
@@ -99,7 +100,7 @@ void do_interrupt(CPUState * env)
 
     if (do_irq) {
         irq_vector = sh_intc_get_pending_vector(env->intc_handle,
-						(env->sr >> 4) & 0xf);
+						&priority);
         if (irq_vector == -1) {
             return; /* masked */
 	}
@@ -157,6 +158,13 @@ void do_interrupt(CPUState * env)
     }
 
     env->ssr = env->sr;
+
+    if (env->opm & (1 << 3)) {
+	unsigned mask = 0xf << 4;
+	env->ssr &= ~mask;
+	env->ssr |= (priority << 4) & mask;
+    }
+
     env->spc = env->pc;
     env->sgr = env->gregs[15];
     env->sr |= SR_BL | SR_MD | SR_RB;
-- 
1.5.6.3

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

[Vladimir Prus]

On Sunday 08 February 2009 21:57:25 takasi-y@ops.dti.ne.jp wrote:
> Hi, Vladimir.
> 
> I managed to make the code working.
> r2d boot, SCI and CF working, and /proc/interrupts increases.
> 
> Essential points modified are below.
> 1. sh_intc_init() caller/callee mismatch.
> 2. nobody calls sh_intc_set_irl_priorities()
> 3. INTC_MODE_DUAL_SET/CLR swapped
> I'm not sure 3 in your patch is on purpose or not.
> 
> Attached patch is a diff against rev#6563.
> This is yours + my small fixes, which are..
> - Above three
> - sh_intc_set_irl_priorities() has switched to sh_intc_init_irl_priorities().
> - sh_intc_set_irl()'s enable hack removed.
> - indent,tab/space,brace changed (to what looks like code around)
> - reduce INTC_A7() usage
> - some others.

Hi Yoshii,
I have replaced my original patch with your patch in my patch set, and adjusted
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.
Here's the relevant code:

	 case INTC_MODE_DUAL_SET: value = *valuep & ~value; break;

This clears the relevant bit in 'value', and then:

	sh_intc_toggle_mask(desc, enum_ids[k], value & mask, priority, 0);

this passes '0' for 'enable' parameter to sh_intc_toggle_mask, like it should.

However, unless I miss something, it seems like the value read from a register
is actually inverted. When reading from a set register we should get 1 for each
masked register, and we seem to get 1 for each enabled register. If you agree
with this analysis, I can adjust the read function.

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.

- Volodya

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/trunk@6626
 + 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 <takasi-y@ops.dti.ne.jp>
---
 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

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

[Vladimir Prus]

[-- Attachment #1: Type: text/plain, Size: 1073 bytes --]

On Tuesday 17 February 2009 21:32:15 takasi-y@ops.dti.ne.jp wrote:
> 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/trunk@6626
>  + Last patch: http://lists.gnu.org/archive/html/qemu-devel/2009-02/msg00546.html
>  + This patch.

Yoshii,

with these patches, I've got lockup again, so I've debugged deeper. The lockup happens
because the TMU0 source was asserted, but not enabled. On 7785, that source is controlled
using:

	- INT2PRI0 register, which is priority register. In debugger, I see write
	to this register being processed, and bumping enable_count.
	- INT2MSKR register (and it's clear register). It's never written.

On the CPU, INT2MSKR is initialized to all zeros, not masking anything. In QEMU,
we set enable_max to 2 and enable_count to 0. Therefore, after INT2PRI0 is written,
processor allows the interrupt, and QEMU does not. I attach a patch to correct
this -- what do you think?

Thanks,
Volodya


- Volodya

[-- Attachment #2: mask_groups.diff --]
[-- Type: text/x-diff, Size: 1295 bytes --]

diff --git a/hw/sh_intc.c b/hw/sh_intc.c
index 7cd1fad..2369e8b 100644
--- a/hw/sh_intc.c
+++ b/hw/sh_intc.c
@@ -367,6 +367,10 @@ static void sh_intc_register_source(struct intc_desc *desc,
 	    for (k = 0; k < ARRAY_SIZE(mr->enum_ids); k++) {
 		if (mr->enum_ids[k] == source) {
 		    s->enable_max++;
+		    // Account for the fact that mask registers are
+		    // zero by default, and therefore don't mask
+		    // an interrupt by default.
+		    s->enable_count++;
 		    break;
 		}
 	    }
@@ -431,6 +435,7 @@ void sh_intc_register_sources(struct intc_desc *desc,
 
 		child = sh_intc_source(desc, gr->enum_ids[k]);
 		child->enable_max += s->enable_max;
+		child->enable_count += s->enable_count;
 	    }
 
 	    /* Chain sources within each group via source->next_enum_id,
@@ -537,3 +542,18 @@ void sh_intc_init_irl_priorities(struct intc_source *s)
 	s->priority = priority-- *2;
     assert(priority == 0);
 }
+
+void sh_intc_debug_lockup (struct intc_desc *desc)
+{
+  int i;
+  for (i = 0; i < desc->nr_sources; i++) 
+    {
+      struct intc_source *source = desc->sources + i;
+
+      if (source->asserted && !source->pending)
+	{
+	  printf ("source %d (%x) asserted, not pending, e = %d/%d\n",
+		  i, source->vect, source->enable_count, source->enable_max);
+	}
+    }
+}

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 14:05 Thu 12 Feb     , Vladimir Prus wrote:
> On Sunday 08 February 2009 21:57:25 takasi-y@ops.dti.ne.jp wrote:
> > Hi, Vladimir.
> > 
> > I managed to make the code working.
> > r2d boot, SCI and CF working, and /proc/interrupts increases.
> > 
> > Essential points modified are below.
> > 1. sh_intc_init() caller/callee mismatch.
> > 2. nobody calls sh_intc_set_irl_priorities()
> > 3. INTC_MODE_DUAL_SET/CLR swapped
> > I'm not sure 3 in your patch is on purpose or not.
> > 
> > Attached patch is a diff against rev#6563.
> > This is yours + my small fixes, which are..
> > - Above three
> > - sh_intc_set_irl_priorities() has switched to sh_intc_init_irl_priorities().
> > - sh_intc_set_irl()'s enable hack removed.
> > - indent,tab/space,brace changed (to what looks like code around)
> > - reduce INTC_A7() usage
> > - some others.
> 
> Hi Yoshii,
> I have replaced my original patch with your patch in my patch set, and adjusted
> sh7785 emulation for your changes. It works, but only after I re-do the
where can we found this famous sh7785 patch set?

Best Regards,
J.

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

[Vladimir Prus]

On Friday 20 February 2009 06:06:57 Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 14:05 Thu 12 Feb     , Vladimir Prus wrote:
> > On Sunday 08 February 2009 21:57:25 takasi-y@ops.dti.ne.jp wrote:
> > > Hi, Vladimir.
> > > 
> > > I managed to make the code working.
> > > r2d boot, SCI and CF working, and /proc/interrupts increases.
> > > 
> > > Essential points modified are below.
> > > 1. sh_intc_init() caller/callee mismatch.
> > > 2. nobody calls sh_intc_set_irl_priorities()
> > > 3. INTC_MODE_DUAL_SET/CLR swapped
> > > I'm not sure 3 in your patch is on purpose or not.
> > > 
> > > Attached patch is a diff against rev#6563.
> > > This is yours + my small fixes, which are..
> > > - Above three
> > > - sh_intc_set_irl_priorities() has switched to sh_intc_init_irl_priorities().
> > > - sh_intc_set_irl()'s enable hack removed.
> > > - indent,tab/space,brace changed (to what looks like code around)
> > > - reduce INTC_A7() usage
> > > - some others.
> > 
> > Hi Yoshii,
> > I have replaced my original patch with your patch in my patch set, and adjusted
> > sh7785 emulation for your changes. It works, but only after I re-do the
> where can we found this famous sh7785 patch set?

Only in my git tree, at the moment. There's one patch there that needs some
rework before it's posted.

- Volodya

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 08:16 Fri 20 Feb     , Vladimir Prus wrote:
> On Friday 20 February 2009 06:06:57 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 14:05 Thu 12 Feb     , Vladimir Prus wrote:
> > > On Sunday 08 February 2009 21:57:25 takasi-y@ops.dti.ne.jp wrote:
> > > > Hi, Vladimir.
> > > > 
> > > > I managed to make the code working.
> > > > r2d boot, SCI and CF working, and /proc/interrupts increases.
> > > > 
> > > > Essential points modified are below.
> > > > 1. sh_intc_init() caller/callee mismatch.
> > > > 2. nobody calls sh_intc_set_irl_priorities()
> > > > 3. INTC_MODE_DUAL_SET/CLR swapped
> > > > I'm not sure 3 in your patch is on purpose or not.
> > > > 
> > > > Attached patch is a diff against rev#6563.
> > > > This is yours + my small fixes, which are..
> > > > - Above three
> > > > - sh_intc_set_irl_priorities() has switched to sh_intc_init_irl_priorities().
> > > > - sh_intc_set_irl()'s enable hack removed.
> > > > - indent,tab/space,brace changed (to what looks like code around)
> > > > - reduce INTC_A7() usage
> > > > - some others.
> > > 
> > > Hi Yoshii,
> > > I have replaced my original patch with your patch in my patch set, and adjusted
> > > sh7785 emulation for your changes. It works, but only after I re-do the
> > where can we found this famous sh7785 patch set?
> 
> Only in my git tree, at the moment. There's one patch there that needs some
> rework before it's posted.
a link maybe?

Best Regards,
J.

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

[Vladimir Prus]

On Friday 20 February 2009 08:32:49 Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 08:16 Fri 20 Feb     , Vladimir Prus wrote:
> > On Friday 20 February 2009 06:06:57 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 14:05 Thu 12 Feb     , Vladimir Prus wrote:
> > > > On Sunday 08 February 2009 21:57:25 takasi-y@ops.dti.ne.jp wrote:
> > > > > Hi, Vladimir.
> > > > > 
> > > > > I managed to make the code working.
> > > > > r2d boot, SCI and CF working, and /proc/interrupts increases.
> > > > > 
> > > > > Essential points modified are below.
> > > > > 1. sh_intc_init() caller/callee mismatch.
> > > > > 2. nobody calls sh_intc_set_irl_priorities()
> > > > > 3. INTC_MODE_DUAL_SET/CLR swapped
> > > > > I'm not sure 3 in your patch is on purpose or not.
> > > > > 
> > > > > Attached patch is a diff against rev#6563.
> > > > > This is yours + my small fixes, which are..
> > > > > - Above three
> > > > > - sh_intc_set_irl_priorities() has switched to sh_intc_init_irl_priorities().
> > > > > - sh_intc_set_irl()'s enable hack removed.
> > > > > - indent,tab/space,brace changed (to what looks like code around)
> > > > > - reduce INTC_A7() usage
> > > > > - some others.
> > > > 
> > > > Hi Yoshii,
> > > > I have replaced my original patch with your patch in my patch set, and adjusted
> > > > sh7785 emulation for your changes. It works, but only after I re-do the
> > > where can we found this famous sh7785 patch set?
> > 
> > Only in my git tree, at the moment. There's one patch there that needs some
> > rework before it's posted.
> a link maybe?

Sorry, I should have said "my local git tree". I can send the entire patch set
to you, if you're interested.

- Volodya

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 08:58 Fri 20 Feb     , Vladimir Prus wrote:
> On Friday 20 February 2009 08:32:49 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 08:16 Fri 20 Feb     , Vladimir Prus wrote:
> > > On Friday 20 February 2009 06:06:57 Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 14:05 Thu 12 Feb     , Vladimir Prus wrote:
> > > > > On Sunday 08 February 2009 21:57:25 takasi-y@ops.dti.ne.jp wrote:
> > > > > > Hi, Vladimir.
> > > > > > 
> > > > > > I managed to make the code working.
> > > > > > r2d boot, SCI and CF working, and /proc/interrupts increases.
> > > > > > 
> > > > > > Essential points modified are below.
> > > > > > 1. sh_intc_init() caller/callee mismatch.
> > > > > > 2. nobody calls sh_intc_set_irl_priorities()
> > > > > > 3. INTC_MODE_DUAL_SET/CLR swapped
> > > > > > I'm not sure 3 in your patch is on purpose or not.
> > > > > > 
> > > > > > Attached patch is a diff against rev#6563.
> > > > > > This is yours + my small fixes, which are..
> > > > > > - Above three
> > > > > > - sh_intc_set_irl_priorities() has switched to sh_intc_init_irl_priorities().
> > > > > > - sh_intc_set_irl()'s enable hack removed.
> > > > > > - indent,tab/space,brace changed (to what looks like code around)
> > > > > > - reduce INTC_A7() usage
> > > > > > - some others.
> > > > > 
> > > > > Hi Yoshii,
> > > > > I have replaced my original patch with your patch in my patch set, and adjusted
> > > > > sh7785 emulation for your changes. It works, but only after I re-do the
> > > > where can we found this famous sh7785 patch set?
> > > 
> > > Only in my git tree, at the moment. There's one patch there that needs some
> > > rework before it's posted.
> > a link maybe?
> 
> Sorry, I should have said "my local git tree". I can send the entire patch set
> to you, if you're interested.
I'm and I think the other SH4 dev too (as Kawasaki-san)

Best Regards,
J.

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

[takasi-y]

Hi,

> On the CPU, INT2MSKR is initialized to all zeros, not masking anything. In QEMU,
Please read HW manual. According to a SH7785 HW manual(REJ09B0261-0100),
Initial value of four mask registers are {ff000000,ff000000,00000000,ffffffff}.
There's no general rule for initial value of registers.
But default = 0 not seems to be a bad idea.
Attached patch enables to supply initial value, otherwise 0.

Tested for little endian r2d on PC with source below
svn head: svn://svn.sv.gnu.org/qemu/trunk@6838
+ --append patch : Sun, 8 Mar 2009 03:00:17 +0900 (JST)
+ intc old patch : Mon, 9 Feb 2009 03:57:25 +0900 (JST)
+ intc last patch : Wed, 18 Feb 2009 03:32:15 +0900 (JST)
+ This patch.

BTW, our(mostly "my", sorry) slow conversation seems to upset some people.
I don't care writing code without your source code, because my reference
is always a HW manual. But, it is true that this style of development is
not efficient nor interesting.

How about committing your patches even if your target doesn't work?
I do care regression, and adding code not based on HW manual.
But your target is new one, it is not a regression ;)

/yoshii
---
Initial value for registers support.
More fix about priority 4<->5 bit conversion.

Signed-off-by: Takashi YOSHII <takasi-y@ops.dti.ne.jp>

diff --git a/hw/sh7750.c b/hw/sh7750.c
index 57a5d7d..7c9bae8 100644
--- a/hw/sh7750.c
+++ b/hw/sh7750.c
@@ -505,7 +505,7 @@ static struct intc_prio_reg prio_registers[] = {
 	{ 0xffd00004, 0, 16, 4, /* IPRA */ { TMU0, TMU1, TMU2, RTC } },
 	{ 0xffd00008, 0, 16, 4, /* IPRB */ { WDT, REF, SCI1, 0 } },
 	{ 0xffd0000c, 0, 16, 4, /* IPRC */ { GPIOI, DMAC, SCIF, HUDI } },
-	{ 0xffd00010, 0, 16, 4, /* IPRD */ { IRL0, IRL1, IRL2, IRL3 } },
+	{ 0xffd00010, 0, 16, 4, /* IPRD */ { IRL0, IRL1, IRL2, IRL3 }, 0xda74 },
 	{ 0xfe080000, 0, 32, 4, /* INTPRI00 */ { 0, 0, 0, 0,
 						 TMU4, TMU3,
 						 PCIC1, PCIC0_PCISERR } },
diff --git a/hw/sh_intc.c b/hw/sh_intc.c
index 7cd1fad..a67d148 100644
--- a/hw/sh_intc.c
+++ b/hw/sh_intc.c
@@ -105,6 +105,9 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int *priority)
 	}
     }
 
+    /* Priority in intc is 5 bits, whereas processor knows only 4 bits. */
+    highest_priority >>= 1;
+
     if (found != -1  && highest_priority > *priority) {
 	struct intc_source *source = desc->sources + found;
 
@@ -115,8 +118,7 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int *priority)
 	if (desc->icr0 & ICR0_LVLMODE)
 	    sh_intc_toggle_source(source, 0, -1);
 
-	/* Priority in intc is 5 bits, whereas processor knows only 4 bits. */
-	*priority = highest_priority >> 1;
+	*priority = highest_priority;
 
 	return source->vect;
     }
@@ -365,8 +367,10 @@ static void sh_intc_register_source(struct intc_desc *desc,
 	    struct intc_mask_reg *mr = desc->mask_regs + i;
 
 	    for (k = 0; k < ARRAY_SIZE(mr->enum_ids); k++) {
+		int shift = mr->reg_width - (k+1);
 		if (mr->enum_ids[k] == source) {
 		    s->enable_max++;
+		    s->enable_count += (mr->value >> shift) & 1;
 		    break;
 		}
 	    }
@@ -378,8 +382,12 @@ static void sh_intc_register_source(struct intc_desc *desc,
 	    struct intc_prio_reg *pr = desc->prio_regs + i;
 
 	    for (k = 0; k < ARRAY_SIZE(pr->enum_ids); k++) {
+		int shift = pr->reg_width - pr->field_width * (k+1);
+		int mask = (1 << pr->field_width) - 1;
 		if (pr->enum_ids[k] == source) {
+		    s->priority = (pr->value >> shift) & mask;
 		    s->enable_max++;
+		    s->enable_count += s->priority?1:0;
 		    break;
 		}
 	    }

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

[Vladimir Prus]

On Friday 13 March 2009 20:50:22 takasi-y@ops.dti.ne.jp wrote:
> Hi,
>
> > On the CPU, INT2MSKR is initialized to all zeros, not masking anything.
> > In QEMU,
>
> Please read HW manual. According to a SH7785 HW manual(REJ09B0261-0100),
> Initial value of four mask registers are
> {ff000000,ff000000,00000000,ffffffff}. There's no general rule for initial
> value of registers.
> But default = 0 not seems to be a bad idea.
> Attached patch enables to supply initial value, otherwise 0.
>
> Tested for little endian r2d on PC with source below
> svn head: svn://svn.sv.gnu.org/qemu/trunk@6838
> + --append patch : Sun, 8 Mar 2009 03:00:17 +0900 (JST)
> + intc old patch : Mon, 9 Feb 2009 03:57:25 +0900 (JST)
> + intc last patch : Wed, 18 Feb 2009 03:32:15 +0900 (JST)
> + This patch.
>
> BTW, our(mostly "my", sorry) slow conversation seems to upset some people.
> I don't care writing code without your source code, 

Hmm, maybe I am confused but I had an impression that you do have access
to the sh4a qemu -- both binary and source. 

> because my reference
> is always a HW manual. But, it is true that this style of development is
> not efficient nor interesting.

Indeed, mailing patches and revisions back and forth is cumbersome. If the
above set of patches works for you for r2d, then maybe the best approach
is to get them checked in -- and then I'll have a baseline to revise my
patch series against?

I assume that core qemu maintainers are not watching this thread closely.
Do you think you can post the above patches -- either combined, or separately,
for review?

Thanks,
Volodya

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

[takasi-y]

Hi, Vladimir,
Sorry for making you be confused,
# I worried when I had been assigned to qemu related job, though....

All development works I have done on qemu are for my hobby.
This mail address, which is used to sign-off for qemu, is for my personal use.

> Hmm, maybe I am confused but I had an impression that you do have access
> to the sh4a qemu -- both binary and source. 
My HDD won't have data that is not publicly available to make license
contamination avoidance easy. Anyway, I can't take any data away from office
to home, because it is prohibited by a company rule.

> Indeed, mailing patches and revisions back and forth is cumbersome. If the
> above set of patches works for you for r2d, then maybe the best approach
> is to get them checked in -- and then I'll have a baseline to revise my
> patch series against?
Perhaps, that will make things easier.

> I assume that core qemu maintainers are not watching this thread closely.
> Do you think you can post the above patches -- either combined, or separately,
> for review?
Developers' test reports are important as well as Maintainer's check.
So, each post should be enough to be build, run, and test the new function.

Being asked if I post it, I think I will rather choose not committing it.
Because
1. No problem has been reported with current code.
This makes "test the new function" above difficult.
2. The based code (current one) itself seems to be problematic.
At least I don't think controlling enable/disable by counter works well,
 especially when there are more than one masking source like INTC2 and INT2.
# So, IPR might be OK.

But, anyway, I don't mind it be checked in, unless it has regressions.
I can fix the problem I mentioned above even after it be checked in.
Or, perhaps will do nothing, because I prefer to spare time, if any,
 to verify and fix CPU core part.

Regards,
/yoshii

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

[Vladimir Prus]

On Thursday 19 March 2009 20:17:38 takasi-y@ops.dti.ne.jp wrote:

Yoshii,

> Sorry for making you be confused,
> # I worried when I had been assigned to qemu related job, though....
> 
> All development works I have done on qemu are for my hobby.
> This mail address, which is used to sign-off for qemu, is for my personal use.

OK.

> > Indeed, mailing patches and revisions back and forth is cumbersome. If the
> > above set of patches works for you for r2d, then maybe the best approach
> > is to get them checked in -- and then I'll have a baseline to revise my
> > patch series against?
> Perhaps, that will make things easier.
...
> Being asked if I post it, I think I will rather choose not committing it.
...
> But, anyway, I don't mind it be checked in, unless it has regressions.
...

It is my understanding that my original patch, plus three revision you have
sent together cause no regression for r2d. At the same time, my original
patch is necessary for sh4a, for reason I have described when I have
posted it. Based on that, I will combine my and your patches, and will
post it early next week.

- Volodya