Hello,
On Wed, Feb 16, 2011 at 4:13 AM, andrzej zaborowski <balr...@gmail.com> wrote:
> On 12 February 2011 01:15, Dmitry Eremin-Solenikov <dbarysh...@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 <dbarysh...@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
