mainstone: correct and simplify irq handling

PATCH In review Dmitry Eremin-Solenikov 2 participants 5 messages Feb 12, 2011 – Feb 16, 2011

lore

Share & tools

lore

Dmitry Eremin-Solenikov

author

2011-02-12 00:15 UTC #0 lore raw

PATCH 1/3 mainstone: correct and simplify irq handling

Simplify IRQ handling to stop setting an input irq pin. As a win, also get correct IRQ status after save/load cycle.

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

---
 hw/mst_fpga.c |   29 ++++++++++-------------------
 1 files changed, 10 insertions(+), 19 deletions(-)

hw/mst_fpga.c +10 ~~-19~~

index 93c6514..3c594b8 100644
@@ -46,33 +46,21 @@ typedef struct mst_irq_state{
 }mst_irq_state;
 
 static void
-mst_fpga_update_gpio(mst_irq_state *s)
-{
-	uint32_t level, diff;
-	int bit;
-	level = s->prev_level ^ s->intsetclr;
-
-	for (diff = s->prev_level ^ level; diff; diff ^= 1 << bit) {
-		bit = ffs(diff) - 1;
-		qemu_set_irq(s->pins[bit], (level >> bit) & 1 );
-	}
-	s->prev_level = level;
-}
-
-static void
 mst_fpga_set_irq(void *opaque, int irq, int level)
 {
 	mst_irq_state *s = (mst_irq_state *)opaque;
+	uint32_t oldint = s->intsetclr;
 
 	if (level)
 		s->prev_level |= 1u << irq;
 	else
 		s->prev_level &= ~(1u << irq);
 
-	if(s->intmskena & (1u << irq)) {
-		s->intsetclr = 1u << irq;
-		qemu_set_irq(s->parent, level);
-	}
+	if ((s->intmskena & (1u << irq)) && level)
+		s->intsetclr |= 1u << irq;
+
+	if (oldint != (s->intsetclr & s->intmskena))
+		qemu_set_irq(s->parent, s->intsetclr & s->intmskena);
 }

@@ -146,10 +134,11 @@ mst_fpga_writeb(void *opaque, target_phys_addr_t addr, uint32_t value)
 		break;
 	case MST_INTMSKENA:	/* Mask interupt */
 		s->intmskena = (value & 0xFEEFF);
-		mst_fpga_update_gpio(s);
+		qemu_set_irq(s->parent, s->intsetclr & s->intmskena);
 		break;
 	case MST_INTSETCLR:	/* clear or set interrupt */
 		s->intsetclr = (value & 0xFEEFF);
+		qemu_set_irq(s->parent, s->intsetclr);
 		break;
 	case MST_PCMCIA0:
 		s->pcmcia0 = value;

@@ -212,6 +201,8 @@ mst_fpga_load(QEMUFile *f, void *opaque, int version_id)
 	qemu_get_be32s(f, &s->intsetclr);
 	qemu_get_be32s(f, &s->pcmcia0);
 	qemu_get_be32s(f, &s->pcmcia1);
+
+	qemu_set_irq(s->parent, s->intsetclr & s->intmskena);
 	return 0;
 }

Signature · 1 line

1.7.2.3

Dmitry Eremin-Solenikov

author

2011-02-12 00:15 UTC #1 lore raw

PATCH 2/3 mainstone: convert FPGA emulation code to use QDev/SysBus

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

---
 hw/mainstone.c |   10 +++--
 hw/mainstone.h |    3 --
 hw/mst_fpga.c  |   96 ++++++++++++++++++++++++++++----------------------------
 3 files changed, 54 insertions(+), 55 deletions(-)

hw/mainstone.c +6 -4

index 18d1415..9c8bc15 100644
@@ -18,6 +18,7 @@
 #include "sysemu.h"
 #include "flash.h"
 #include "blockdev.h"
+#include "sysbus.h"
 
 static struct keymap map[0xE0] = {
     [0 ... 0xDF] = { -1, -1 },

@@ -77,7 +78,7 @@ static void mainstone_common_init(ram_addr_t ram_size,
     uint32_t sector_len = 256 * 1024;
     target_phys_addr_t mainstone_flash_base[] = { MST_FLASH_0, MST_FLASH_1 };
     PXA2xxState *cpu;
-    qemu_irq *mst_irq;
+    DeviceState *mst_irq;
     DriveInfo *dinfo;
     int i;
     int be;

@@ -117,16 +118,17 @@ static void mainstone_common_init(ram_addr_t ram_size,
         }
     }
 
-    mst_irq = mst_irq_init(MST_FPGA_PHYS, cpu->pic[PXA2XX_PIC_GPIO_0]);
+    mst_irq = sysbus_create_simple("mainstone-fpga", MST_FPGA_PHYS,
+                    cpu->pic[PXA2XX_PIC_GPIO_0]);
 
     /* setup keypad */
     printf("map addr %p\n", &map);
     pxa27x_register_keypad(cpu->kp, map, 0xe0);
 
     /* MMC/SD host */
-    pxa2xx_mmci_handlers(cpu->mmc, NULL, mst_irq[MMC_IRQ]);
+    pxa2xx_mmci_handlers(cpu->mmc, NULL, qdev_get_gpio_in(mst_irq, MMC_IRQ));
 
-    smc91c111_init(&nd_table[0], MST_ETH_PHYS, mst_irq[ETHERNET_IRQ]);
+    smc91c111_init(&nd_table[0], MST_ETH_PHYS, qdev_get_gpio_in(mst_irq, ETHERNET_IRQ));
 
     mainstone_binfo.kernel_filename = kernel_filename;
     mainstone_binfo.kernel_cmdline = kernel_cmdline;

hw/mainstone.h +0 -3

index 35329f1..e6a2b67 100644
@@ -32,7 +32,4 @@
 #define S1_STSCHG_IRQ 14
 #define S1_IRQ        15
 
-extern qemu_irq
-*mst_irq_init(uint32_t base, qemu_irq irq);
-
 #endif /* __MAINSTONE_H__ */

hw/mst_fpga.c +48 ~~-48~~

131 lines not on this page · raw message

Signature · 1 line

1.7.2.3

Dmitry Eremin-Solenikov

author

2011-02-12 00:15 UTC #2 lore raw

PATCH 3/3 Merge mainstone.h header into mainstone.c

In reply to Dmitry Eremin-Solenikov

Now the only user of mainstone.h is mainstone.c file. Merge header into board file.

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>

---
 hw/mainstone.c |   23 ++++++++++++++++++++++-
 hw/mainstone.h |   35 -----------------------------------
 2 files changed, 22 insertions(+), 36 deletions(-)
 delete mode 100644 hw/mainstone.h

hw/mainstone.c +22 -1

index 9c8bc15..41c1433 100644
@@ -14,12 +14,33 @@
 #include "net.h"
 #include "devices.h"
 #include "boards.h"
-#include "mainstone.h"
 #include "sysemu.h"
 #include "flash.h"
 #include "blockdev.h"
 #include "sysbus.h"
 
+/* Device addresses */
+#define MST_FPGA_PHYS	0x08000000
+#define MST_ETH_PHYS	0x10000300
+#define MST_FLASH_0		0x00000000
+#define MST_FLASH_1		0x04000000
+
+/* IRQ definitions */
+#define MMC_IRQ       0
+#define USIM_IRQ      1
+#define USBC_IRQ      2
+#define ETHERNET_IRQ  3
+#define AC97_IRQ      4
+#define PEN_IRQ       5
+#define MSINS_IRQ     6
+#define EXBRD_IRQ     7
+#define S0_CD_IRQ     9
+#define S0_STSCHG_IRQ 10
+#define S0_IRQ        11
+#define S1_CD_IRQ     13
+#define S1_STSCHG_IRQ 14
+#define S1_IRQ        15
+
 static struct keymap map[0xE0] = {
     [0 ... 0xDF] = { -1, -1 },
     [0x1e] = {0,0}, /* a */

hw/mainstone.h deleted +0 ~~-35~~

deleted file mode 100644
index e6a2b67..0000000
@@ -1,35 +0,0 @@
-/*
- * PXA270-based Intel Mainstone platforms.
- *
- * Copyright (c) 2007 by Armin Kuster <[email protected]> or
- *                                    <[email protected]>
- *
- * This code is licensed under the GNU GPL v2.
- */
-
-#ifndef __MAINSTONE_H__
-#define __MAINSTONE_H__
-
-/* Device addresses */
-#define MST_FPGA_PHYS	0x08000000
-#define MST_ETH_PHYS	0x10000300
-#define MST_FLASH_0		0x00000000
-#define MST_FLASH_1		0x04000000
-
-/* IRQ definitions */
-#define MMC_IRQ       0
-#define USIM_IRQ      1
-#define USBC_IRQ      2
-#define ETHERNET_IRQ  3
-#define AC97_IRQ      4
-#define PEN_IRQ       5
-#define MSINS_IRQ     6
-#define EXBRD_IRQ     7
-#define S0_CD_IRQ     9
-#define S0_STSCHG_IRQ 10
-#define S0_IRQ        11
-#define S1_CD_IRQ     13
-#define S1_STSCHG_IRQ 14
-#define S1_IRQ        15
-
-#endif /* __MAINSTONE_H__ */

Signature · 1 line

1.7.2.3

andrzej zaborowski

2011-02-16 01:13 UTC #3 lore raw

In reply to Dmitry Eremin-Solenikov

Quoted 2 lines · Dmitry Eremin-Solenikov

Simplify IRQ handling to stop setting an input irq pin. As a win, also get correct IRQ status after save/load cycle.

Thanks, I pushed the three patches from you but see a question below.

Quoted 38 lines

Signed-off-by Dmitry Eremin-Solenikov <[email protected]>
---
hw/mst_fpga.c | 29 ++++++++++-------------------
 1 files changed, 10 insertions(+), 19 deletions(-)
hw/mst_fpga.c +0 -0
index 93c6514..3c594b8 100644
@@ -46,33 +46,21 @@ typedef struct mst_irq_state{
  }mst_irq_state;
 
static void
hw/mst_fpga.c +0 ~~-14~~
-mst_fpga_update_gpio(mst_irq_state *s)
-{
-       uint32_t level, diff;
-       int bit;
-       level = s->prev_level ^ s->intsetclr;
-
-       for (diff = s->prev_level ^ level; diff; diff ^= 1 << bit) {
-               bit = ffs(diff) - 1;
-               qemu_set_irq(s->pins[bit], (level >> bit) & 1 );
-       }
-       s->prev_level = level;
-}
-
-static void
mst_fpga_set_irq(void *opaque, int irq, int level) {
mst_irq_state *s = (mst_irq_state *)opaque;
+ uint32_t oldint = s->intsetclr;
if (level)
s->prev_level |= 1u << irq;
else
s->prev_level &= ~(1u << irq);

Quoted 9 lines

- if(s->intmskena & (1u << irq)) {
- s->intsetclr = 1u << irq;
- qemu_set_irq(s->parent, level);
- }
+ if ((s->intmskena & (1u << irq)) && level)
+ s->intsetclr |= 1u << irq;
+
+ if (oldint != (s->intsetclr & s->intmskena))
+ qemu_set_irq(s->parent, s->intsetclr & s->intmskena);

Shouldn't this looks something like:

oldint = s->intsetclr & s->intmskena;
if (level)
s->intsetclr |= 1 << irq;
if (oldint != (s->intsetclr & s->intmskena))

    qemu_set_irq(s->parent, s->intsetclr & s->intmskena);

I don't know this device but this is the usual outline.

Quoted 5 lines

}

hw/mst_fpga.c +0 -0

@@ -146,10 +134,11 @@ mst_fpga_writeb(void *opaque, target_phys_addr_t addr, uint32_t value)
                break;

Quoted 8 lines

case MST_INTMSKENA: /* Mask interupt */ s->intmskena = (value & 0xFEEFF);
- mst_fpga_update_gpio(s);
+ qemu_set_irq(s->parent, s->intsetclr & s->intmskena); break;
case MST_INTSETCLR: /* clear or set interrupt */ s->intsetclr = (value & 0xFEEFF);
+ qemu_set_irq(s->parent, s->intsetclr);

Shouldn't this also be masked?

Cheers

Dmitry Eremin-Solenikov

author

2011-02-16 12:49 UTC #4 lore raw

Hello,

Quoted 56 lines · andrzej zaborowski

Quoted 2 lines · Dmitry Eremin-Solenikov
Simplify IRQ handling to stop setting an input irq pin. As a win, also get correct IRQ status after save/load cycle.
Thanks, I pushed the three patches from you but see a question below.
Quoted 38 lines
Signed-off-by Dmitry Eremin-Solenikov <[email protected]>
---
hw/mst_fpga.c | 29 ++++++++++-------------------
 1 files changed, 10 insertions(+), 19 deletions(-)
hw/mst_fpga.c +0 -0
index 93c6514..3c594b8 100644
@@ -46,33 +46,21 @@ typedef struct mst_irq_state{
  }mst_irq_state;
 
static void
hw/mst_fpga.c +0 ~~-14~~
-mst_fpga_update_gpio(mst_irq_state *s)
-{
-       uint32_t level, diff;
-       int bit;
-       level = s->prev_level ^ s->intsetclr;
-
-       for (diff = s->prev_level ^ level; diff; diff ^= 1 << bit) {
-               bit = ffs(diff) - 1;
-               qemu_set_irq(s->pins[bit], (level >> bit) & 1 );
-       }
-       s->prev_level = level;
-}
-
-static void
mst_fpga_set_irq(void *opaque, int irq, int level) {
mst_irq_state *s = (mst_irq_state *)opaque;
+ uint32_t oldint = s->intsetclr;
if (level)
s->prev_level |= 1u << irq;
else
s->prev_level &= ~(1u << irq);
Quoted 9 lines
- if(s->intmskena & (1u << irq)) {
- s->intsetclr = 1u << irq;
- qemu_set_irq(s->parent, level);
- }
+ if ((s->intmskena & (1u << irq)) && level)
+ s->intsetclr |= 1u << irq;
+
+ if (oldint != (s->intsetclr & s->intmskena))
+ qemu_set_irq(s->parent, s->intsetclr & s->intmskena);
Shouldn't this looks something like:

Quoted 7 lines

oldint = s->intsetclr & s->intmskena;
if (level)
s->intsetclr |= 1 << irq;
if (oldint != (s->intsetclr & s->intmskena)) qemu_set_irq(s->parent, s->intsetclr & s->intmskena);
I don't know this device but this is the usual outline.

Maybe. This should not matter really hard, as we set the correct irq level at qemu_set_irq()

Quoted 4 lines

Quoted 4 lines
}

Quoted 12 lines

Quoted 10 lines
hw/mst_fpga.c +0 -0
@@ -146,10 +134,11 @@ mst_fpga_writeb(void *opaque, target_phys_addr_t addr, uint32_t value)
                break;
case MST_INTMSKENA: /* Mask interupt */ s->intmskena = (value & 0xFEEFF);
- mst_fpga_update_gpio(s);
+ qemu_set_irq(s->parent, s->intsetclr & s->intmskena); break;
case MST_INTSETCLR: /* clear or set interrupt */ s->intsetclr = (value & 0xFEEFF);
+ qemu_set_irq(s->parent, s->intsetclr);
Shouldn't this also be masked?

Hmmm. Looks like yes. I'll send the patch.

Signature · 2 lines

With best wishes
Dmitry