Patchwork [1/3] mainstone: correct and simplify irq handling

login
register
mail settings
Submitter Dmitry Eremin-Solenikov
Date Feb. 12, 2011, 12:15 a.m.
Message ID <1297469725-31730-1-git-send-email-dbaryshkov@gmail.com>
Download mbox | patch
Permalink /patch/82879/
State New
Headers show

Comments

Dmitry Eremin-Solenikov - Feb. 12, 2011, 12:15 a.m.
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(-)
andrzej zaborowski - Feb. 16, 2011, 1:13 a.m.
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
Dmitry Eremin-Solenikov - Feb. 16, 2011, 12:49 p.m.
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.

Patch

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;
 }