[Qemu-devel] [PATCH 1/3] mainstone: correct and simplify irq handling

[Qemu-devel] [PATCH 1/3] mainstone: correct and simplify irq handling

[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.

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 hw/mst_fpga.c |   29 ++++++++++-------------------
 1 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/hw/mst_fpga.c b/hw/mst_fpga.c
index 93c6514..3c594b8 100644
--- a/hw/mst_fpga.c
+++ b/hw/mst_fpga.c
@@ -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;
 }
 
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/3] mainstone: convert FPGA emulation code to use QDev/SysBus

[Dmitry Eremin-Solenikov]

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 hw/mainstone.c |   10 +++--
 hw/mainstone.h |    3 --
 hw/mst_fpga.c  |   96 ++++++++++++++++++++++++++++----------------------------
 3 files changed, 54 insertions(+), 55 deletions(-)

diff --git a/hw/mainstone.c b/hw/mainstone.c
index 18d1415..9c8bc15 100644
--- a/hw/mainstone.c
+++ b/hw/mainstone.c
@@ -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;
diff --git a/hw/mainstone.h b/hw/mainstone.h
index 35329f1..e6a2b67 100644
--- a/hw/mainstone.h
+++ b/hw/mainstone.h
@@ -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__ */
diff --git a/hw/mst_fpga.c b/hw/mst_fpga.c
index 3c594b8..afed2ac 100644
--- a/hw/mst_fpga.c
+++ b/hw/mst_fpga.c
@@ -8,7 +8,7 @@
  * This code is licensed under the GNU GPL v2.
  */
 #include "hw.h"
-#include "mainstone.h"
+#include "sysbus.h"
 
 /* Mainstone FPGA for extern irqs */
 #define FPGA_GPIO_PIN	0
@@ -27,8 +27,9 @@
 #define MST_PCMCIA1		0xe4
 
 typedef struct mst_irq_state{
+	SysBusDevice busdev;
+
 	qemu_irq parent;
-	qemu_irq *pins;
 
 	uint32_t prev_level;
 	uint32_t leddat1;
@@ -163,68 +164,67 @@ static CPUWriteMemoryFunc * const mst_fpga_writefn[] = {
 	mst_fpga_writeb,
 };
 
-static void
-mst_fpga_save(QEMUFile *f, void *opaque)
-{
-	struct mst_irq_state *s = (mst_irq_state *) opaque;
-
-	qemu_put_be32s(f, &s->prev_level);
-	qemu_put_be32s(f, &s->leddat1);
-	qemu_put_be32s(f, &s->leddat2);
-	qemu_put_be32s(f, &s->ledctrl);
-	qemu_put_be32s(f, &s->gpswr);
-	qemu_put_be32s(f, &s->mscwr1);
-	qemu_put_be32s(f, &s->mscwr2);
-	qemu_put_be32s(f, &s->mscwr3);
-	qemu_put_be32s(f, &s->mscrd);
-	qemu_put_be32s(f, &s->intmskena);
-	qemu_put_be32s(f, &s->intsetclr);
-	qemu_put_be32s(f, &s->pcmcia0);
-	qemu_put_be32s(f, &s->pcmcia1);
-}
 
-static int
-mst_fpga_load(QEMUFile *f, void *opaque, int version_id)
+static int mst_fpga_post_load(void *opaque, int version_id)
 {
 	mst_irq_state *s = (mst_irq_state *) opaque;
 
-	qemu_get_be32s(f, &s->prev_level);
-	qemu_get_be32s(f, &s->leddat1);
-	qemu_get_be32s(f, &s->leddat2);
-	qemu_get_be32s(f, &s->ledctrl);
-	qemu_get_be32s(f, &s->gpswr);
-	qemu_get_be32s(f, &s->mscwr1);
-	qemu_get_be32s(f, &s->mscwr2);
-	qemu_get_be32s(f, &s->mscwr3);
-	qemu_get_be32s(f, &s->mscrd);
-	qemu_get_be32s(f, &s->intmskena);
-	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;
 }
 
-qemu_irq *mst_irq_init(uint32_t base, qemu_irq irq)
+static int mst_fpga_init(SysBusDevice *dev)
 {
 	mst_irq_state *s;
 	int iomemtype;
-	qemu_irq *qi;
 
-	s = (mst_irq_state  *)
-		qemu_mallocz(sizeof(mst_irq_state));
+	s = FROM_SYSBUS(mst_irq_state, dev);
 
-	s->parent = irq;
+	sysbus_init_irq(dev, &s->parent);
 
 	/* alloc the external 16 irqs */
-	qi  = qemu_allocate_irqs(mst_fpga_set_irq, s, MST_NUM_IRQS);
-	s->pins = qi;
+	qdev_init_gpio_in(&dev->qdev, mst_fpga_set_irq, MST_NUM_IRQS);
 
 	iomemtype = cpu_register_io_memory(mst_fpga_readfn,
 		mst_fpga_writefn, s, DEVICE_NATIVE_ENDIAN);
-	cpu_register_physical_memory(base, 0x00100000, iomemtype);
-	register_savevm(NULL, "mainstone_fpga", 0, 0, mst_fpga_save,
-                        mst_fpga_load, s);
-	return qi;
+	sysbus_init_mmio(dev, 0x00100000, iomemtype);
+	return 0;
+}
+
+static VMStateDescription vmstate_mst_fpga_regs = {
+	.name = "mainstone_fpga",
+	.version_id = 0,
+	.minimum_version_id = 0,
+	.minimum_version_id_old = 0,
+	.post_load = mst_fpga_post_load,
+	.fields = (VMStateField []) {
+		VMSTATE_UINT32(prev_level, mst_irq_state),
+		VMSTATE_UINT32(leddat1, mst_irq_state),
+		VMSTATE_UINT32(leddat2, mst_irq_state),
+		VMSTATE_UINT32(ledctrl, mst_irq_state),
+		VMSTATE_UINT32(gpswr, mst_irq_state),
+		VMSTATE_UINT32(mscwr1, mst_irq_state),
+		VMSTATE_UINT32(mscwr2, mst_irq_state),
+		VMSTATE_UINT32(mscwr3, mst_irq_state),
+		VMSTATE_UINT32(mscrd, mst_irq_state),
+		VMSTATE_UINT32(intmskena, mst_irq_state),
+		VMSTATE_UINT32(intsetclr, mst_irq_state),
+		VMSTATE_UINT32(pcmcia0, mst_irq_state),
+		VMSTATE_UINT32(pcmcia1, mst_irq_state),
+		VMSTATE_END_OF_LIST(),
+	},
+};
+
+static SysBusDeviceInfo mst_fpga_info = {
+	.init = mst_fpga_init,
+	.qdev.name = "mainstone-fpga",
+	.qdev.desc = "Mainstone II FPGA",
+	.qdev.size = sizeof(mst_irq_state),
+	.qdev.vmsd = &vmstate_mst_fpga_regs,
+};
+
+static void mst_fpga_register(void)
+{
+	sysbus_register_withprop(&mst_fpga_info);
 }
+device_init(mst_fpga_register);
-- 
1.7.2.3

[Qemu-devel] [PATCH 3/3] Merge mainstone.h header into mainstone.c

[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 <dbaryshkov@gmail.com>
---
 hw/mainstone.c |   23 ++++++++++++++++++++++-
 hw/mainstone.h |   35 -----------------------------------
 2 files changed, 22 insertions(+), 36 deletions(-)
 delete mode 100644 hw/mainstone.h

diff --git a/hw/mainstone.c b/hw/mainstone.c
index 9c8bc15..41c1433 100644
--- a/hw/mainstone.c
+++ b/hw/mainstone.c
@@ -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 */
diff --git a/hw/mainstone.h b/hw/mainstone.h
deleted file mode 100644
index e6a2b67..0000000
--- a/hw/mainstone.h
+++ /dev/null
@@ -1,35 +0,0 @@
-/*
- * PXA270-based Intel Mainstone platforms.
- *
- * Copyright (c) 2007 by Armin Kuster <akuster@kama-aina.net> or
- *                                    <akuster@mvista.com>
- *
- * 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__ */
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 1/3] mainstone: correct and simplify irq handling

[andrzej zaborowski]

On 12 February 2011 01:15, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote:
> 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.

>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> ---
>  hw/mst_fpga.c |   29 ++++++++++-------------------
>  1 files changed, 10 insertions(+), 19 deletions(-)
>
> diff --git a/hw/mst_fpga.c b/hw/mst_fpga.c
> index 93c6514..3c594b8 100644
> --- a/hw/mst_fpga.c
> +++ b/hw/mst_fpga.c
> @@ -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);

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.

>  }
>
>
> @@ -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?

Cheers

Re: [Qemu-devel] [PATCH 1/3] mainstone: correct and simplify irq handling

[Dmitry Eremin-Solenikov]

Hello,

On Wed, Feb 16, 2011 at 4:13 AM, andrzej zaborowski <balrogg@gmail.com> wrote:
> On 12 February 2011 01:15, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote:
>> 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.
>
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>> ---
>>  hw/mst_fpga.c |   29 ++++++++++-------------------
>>  1 files changed, 10 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/mst_fpga.c b/hw/mst_fpga.c
>> index 93c6514..3c594b8 100644
>> --- a/hw/mst_fpga.c
>> +++ b/hw/mst_fpga.c
>> @@ -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);
>
> 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.

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


>
>>  }
>>
>>
>> @@ -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.

-- 
With best wishes
Dmitry