| Message ID | 1297469725-31730-1-git-send-email-dbaryshkov@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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; }
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(-)