On 28 January 2014 02:09, Christoffer Dall <christoffer.d...@linaro.org> wrote: > On Sun, Jan 19, 2014 at 07:49:58PM +0000, Peter Maydell wrote: >> So this means we now have two lots of pending state, the pending >> and sw_pending fields. I think this is not in fact correct, and there >> is only one lot of state in a real h/w GIC (it's that flipflop). AIUI the >> correct behaviour is: >> >> * GIC_TEST_PENDING() should read: >> ((s->irq_state[irq].pending & (cm) != 0) || >> (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_LEVEL(irq, cm)) >> -- this corresponds to the OR gate in fig 4-10; this is the value >> to be used in reads from ICPENDR/ISPENDR and other "is this >> interrupt pending" checks like the one in gic_update() >> * in gic_set_irq(), if level is 1 (rising edge) we GIC_SET_PENDING >> only if the interrupt is edge-triggered (pending status isn't latched >> for level triggered interrupts). We always GIC_SET_LEVEL(irq, 1). >> * in gic_set_irq(), if level is 0 (falling edge) we don't touch the pending >> state, but just GIC_SET_LEVEL(irq, 0) >> * gic_acknowledge_irq() always does a GIC_CLEAR_PENDING() >> (this won't actually make a level-triggered interrupt be not pending >> because of the second clause of our updated GIC_TEST_PENDING) >> * writes to the I[CS]PENDR registers always result in GIC_SET_PENDING >> or GIC_CLEAR_PENDING calls (updating the latch state) > > ok, it's probably cleaner code-wise, but should produce the same result > as far as I can see. I guess I see the pending field as the result of > the final or-gate (possibly because I'm too tied to the way the > in-kernel emulation works where we don't have a separate 'level' state). > Anyway, I'll respin the patch according to your directions (my only > concern with that is that it feels a bit unlogical to have a bitfield > called pending where some of the bits are not set despite the > corresponding interrupts being pending, but we'll see if the overall > code looks more clean with this approach).
The key thing is that we definitely shouldn't be keeping more state than the hardware does; that's always a bad sign. thanks -- PMM