[Qemu-devel] [PATCH] TSC2005 interrupt handling fix

[Qemu-devel] [PATCH] TSC2005 interrupt handling fix

[Riihimaki Juha (Nokia-D-MSW/Helsinki)]

Fixing couple of timing related issues in the TSC2005 emulation.

Problems:
1) Setting DAV bits while previous conversion results are being read  
results in PINTDAV interrupt line never being released.
2) Altering PINTDAV state during SPI data transfer can result in an  
inconsistent interrupt state.

Solution proposal:
Moved PINTDAV altering code from tsc2005_pin_update to  
tsc2005_timer_tick and prohibited setting of DAV bits unless PINTDAV  
is to be set.


Index: hw/tsc2005.c
===================================================================
--- hw/tsc2005.c	(revision 6098)
+++ hw/tsc2005.c	(working copy)
@@ -230,26 +230,7 @@
  static void tsc2005_pin_update(struct tsc2005_state_s *s)
  {
      int64_t expires;
-    int pin_state;

-    switch (s->pin_func) {
-    case 0:
-        pin_state = !s->pressure && !!s->dav;
-        break;
-    case 1:
-    case 3:
-    default:
-        pin_state = !s->dav;
-        break;
-    case 2:
-        pin_state = !s->pressure;
-    }
-
-    if (pin_state != s->irq) {
-        s->irq = pin_state;
-        qemu_set_irq(s->pint, s->irq);
-    }
-
      switch (s->nextfunction) {
      case TSC_MODE_XYZ_SCAN:
      case TSC_MODE_XY_SCAN:
@@ -401,16 +382,35 @@
  static void tsc2005_timer_tick(void *opaque)
  {
      struct tsc2005_state_s *s = opaque;
+	int pin_state;

      /* Timer ticked -- a set of conversions has been finished.  */

      if (!s->busy)
          return;

-    s->busy = 0;
-    s->dav |= mode_regs[s->function];
-    s->function = -1;
-    tsc2005_pin_update(s);
+	switch (s->pin_func) {
+		case 0:
+			pin_state = !s->pressure && !!s->dav;
+			break;
+		case 1:
+		case 3:
+		default:
+			pin_state = !s->dav;
+			break;
+		case 2:
+			pin_state = !s->pressure;
+    }
+	
+	s->busy = 0;
+	if (pin_state && !s->irq) s->dav |= mode_regs[s->function];
+	s->function = -1;
+	tsc2005_pin_update(s);
+
+    if (pin_state != s->irq) {
+        s->irq = pin_state;
+        qemu_set_irq(s->pint, s->irq);
+    }
  }

  static void tsc2005_touchscreen_event(void *opaque,

Re: [Qemu-devel] [PATCH] TSC2005 interrupt handling fix

[andrzej zaborowski]

2008/12/19 Riihimaki Juha (Nokia-D-MSW/Helsinki) <juha.riihimaki@nokia.com>:
> Fixing couple of timing related issues in the TSC2005 emulation.
>
> Problems:
> 1) Setting DAV bits while previous conversion results are being read results
> in PINTDAV interrupt line never being released.
> 2) Altering PINTDAV state during SPI data transfer can result in an
> inconsistent interrupt state.

Is that because the read happens in two steps (write register number,
read value)?

The specs say that the DAV bit is not reset, and new conversion
doesn't start until the result is completely read out, I assumed that
means it starts after the read command is written, rather than after
both steps finished.  If this wrong, then I see how this can cause
problems.

>
> Solution proposal:
> Moved PINTDAV altering code from tsc2005_pin_update to tsc2005_timer_tick
> and prohibited setting of DAV bits unless PINTDAV is to be set.

You moved the qemu_set_irq to the timer callback while the irq can be
set/reset in other situations too (e.g. on reset). I thinkt he correct
thing to do might be to leave tsc2005_pin_update as it and not reset
the dav bit immediately (adn thus not deassert PINTDAV) until the
value is also fully read.  Only then we would assign s->dav and call
tsc2005_pin_update again and start a new conversion.

Same might apply to tsc2102.

Cheers

Re: [Qemu-devel] [PATCH] TSC2005 interrupt handling fix

[Riihimaki Juha (Nokia-D-MSW/Helsinki)]

Both issues are timing related; it is possible to get a timer tick  
callback in the TSC2005 while executing the guest ISR that is handling  
the interrupt raised by the TSC2005 earlier.

For issue #1 specifically, when this happens with the current version,  
all DAV bits for the running conversion function are set by the timer  
tick callback even if some of the results had already been read out  
and the corresponding DAV bits cleared. Now, when the guest ISR is  
finished and it clears the IRQ, some DAV bits in the TSC2005 will  
remain set and this leads to the TSC2005 never to raise a new  
interrupt again (at least with function 1 in use).

Issue #2 is similar but happens more rarely as it requires more  
precise conditions; if the guest ISR has just finished reading the  
conversion results, i.e. DAV bits are all clear, the SPI read function  
in TSC2005 will immediately clear the interrupt flag. It is then  
possible to get the timer tick callback between this moment and the  
moment when the guest ISR has actually cleared the IRQ. When this  
happens, the callback will try to raise a new interrupt but as the  
guest ISR will clear it shortly thereafter the actual IRQ will be  
cleared but the IRQ flag in the TSC2005 code will stay active causing  
the TSC2005 not to generate new interrupts anymore.

A more elaborate solution than my proposed patch would of course be  
better. I tested my patch only with the "n810" machine emulation with  
an appropriate guest software image which seems to be the only  
configuration using the TSC2005. However I am aware that this  
combination is not utilizing every aspect of the TSC2005.


Regards,
Juha

On Jan 5, 2009, at 07:33, ext andrzej zaborowski wrote:

> 2008/12/19 Riihimaki Juha (Nokia-D-MSW/Helsinki) <juha.riihimaki@nokia.com 
> >:
>> Fixing couple of timing related issues in the TSC2005 emulation.
>>
>> Problems:
>> 1) Setting DAV bits while previous conversion results are being  
>> read results
>> in PINTDAV interrupt line never being released.
>> 2) Altering PINTDAV state during SPI data transfer can result in an
>> inconsistent interrupt state.
>
> Is that because the read happens in two steps (write register number,
> read value)?
>
> The specs say that the DAV bit is not reset, and new conversion
> doesn't start until the result is completely read out, I assumed that
> means it starts after the read command is written, rather than after
> both steps finished.  If this wrong, then I see how this can cause
> problems.
>
>>
>> Solution proposal:
>> Moved PINTDAV altering code from tsc2005_pin_update to  
>> tsc2005_timer_tick
>> and prohibited setting of DAV bits unless PINTDAV is to be set.
>
> You moved the qemu_set_irq to the timer callback while the irq can be
> set/reset in other situations too (e.g. on reset). I thinkt he correct
> thing to do might be to leave tsc2005_pin_update as it and not reset
> the dav bit immediately (adn thus not deassert PINTDAV) until the
> value is also fully read.  Only then we would assign s->dav and call
> tsc2005_pin_update again and start a new conversion.
>
> Same might apply to tsc2102.
>
> Cheers
>
>

Re: [Qemu-devel] [PATCH] TSC2005 interrupt handling fix

[andrzej zaborowski]

2009/1/7 Riihimaki Juha (Nokia-D-MSW/Helsinki) <juha.riihimaki@nokia.com>:
> Both issues are timing related; it is possible to get a timer tick callback
> in the TSC2005 while executing the guest ISR that is handling the interrupt
> raised by the TSC2005 earlier.
>
> For issue #1 specifically, when this happens with the current version, all
> DAV bits for the running conversion function are set by the timer tick
> callback even if some of the results had already been read out and the
> corresponding DAV bits cleared. Now, when the guest ISR is finished and it
> clears the IRQ, some DAV bits in the TSC2005 will remain set and this leads
> to the TSC2005 never to raise a new interrupt again (at least with function
> 1 in use).

The ISR doesn't clear the IRQ -- it can only clear the irq mask flag.
If the TSC2005 still keeps the IRQ pin high, then the cpu should
immediately jump to the ISR again.

We shouldn't be trying to change the tsc2005 emulation so that the
driver works, we should change it so that it behaves as described in
the specification from TI.  I think the real issue is that we start a
new conversion before the irq is deasserted, i.e. the following check
in tscXXXX_pin_update

    if (!s->enabled || s->busy)
        return;

should become

    if (!s->enabled || s->busy || s->dav)
        return;

or something similar.  It would be good to check on the real tsc2005
when exactly the DAC becomes busy (bit 14 of CFR0) in the different
modes.

>
> Issue #2 is similar but happens more rarely as it requires more precise
> conditions; if the guest ISR has just finished reading the conversion
> results, i.e. DAV bits are all clear, the SPI read function in TSC2005 will
> immediately clear the interrupt flag. It is then possible to get the timer
> tick callback between this moment and the moment when the guest ISR has
> actually cleared the IRQ. When this happens, the callback will try to raise
> a new interrupt but as the guest ISR will clear it shortly thereafter the
> actual IRQ will be cleared but the IRQ flag in the TSC2005 code will stay
> active causing the TSC2005 not to generate new interrupts anymore.
>
> A more elaborate solution than my proposed patch would of course be better.
> I tested my patch only with the "n810" machine emulation with an appropriate
> guest software image which seems to be the only configuration using the
> TSC2005. However I am aware that this combination is not utilizing every
> aspect of the TSC2005.

The initial patch you sent was breaking the logic described in the
specs so imho it would be a regression.  The specs says clearly when
the PINT/DAV must be raised and lowered and it's not only in the timer
callback.

Cheers

Re: [Qemu-devel] [PATCH] TSC2005 interrupt handling fix

[Riihimaki Juha (Nokia-D-MSW/Helsinki)]

On Jan 8, 2009, at 09:27, ext andrzej zaborowski wrote:

>> For issue #1 specifically, when this happens with the current  
>> version, all
>> DAV bits for the running conversion function are set by the timer  
>> tick
>> callback even if some of the results had already been read out and  
>> the
>> corresponding DAV bits cleared. Now, when the guest ISR is finished  
>> and it
>> clears the IRQ, some DAV bits in the TSC2005 will remain set and  
>> this leads
>> to the TSC2005 never to raise a new interrupt again (at least with  
>> function
>> 1 in use).
>
> The ISR doesn't clear the IRQ -- it can only clear the irq mask flag.
> If the TSC2005 still keeps the IRQ pin high, then the cpu should
> immediately jump to the ISR again.

True; in the scenario I used for testing PINTDAV is programmed as DAV  
(CFR2 bit 14 is set) and thus the ISR tries to deassert the interrupt  
condition at the TSC2005 by reading all conversion results. It is of  
course the TSC2005 itself that needs to actually clear the interrupt.  
However in the condition I described above, the ISR will not try to  
read new conversion results from the TSC2005 if the interrupt has not  
been cleared in between -- which will not happen if some of the DAV  
bits stay set. This is how it looks like from the TSC2005's viewpoint,  
I have not read the ISR code. Neither have I checked whether the  
problem is that the ISR is not executed at all or if it for some  
reason just refuses to talk with the TSC2005.

> We shouldn't be trying to change the tsc2005 emulation so that the
> driver works, we should change it so that it behaves as described in
> the specification from TI.  I think the real issue is that we start a
> new conversion before the irq is deasserted, i.e. the following check
> in tscXXXX_pin_update
>
>    if (!s->enabled || s->busy)
>        return;
>
> should become
>
>    if (!s->enabled || s->busy || s->dav)
>        return;
>
> or something similar.  It would be good to check on the real tsc2005
> when exactly the DAC becomes busy (bit 14 of CFR0) in the different
> modes.

My original change to overcame this was also a small change; I only  
changed the setting of the DAV bits in the timer callback function to  
set the bits only if the bits for the current function were all clear.  
That had similar effect to the change you are proposing here however  
issue #2 remains still. Perhaps it's just my eyes but I did not find  
any information on TI's spec on the exact timing of the interrupt  
line, for example the time between reading the last available data  
value and when the interrupt is cleared and how quickly after that the  
interrupt line can be raised again.


Regards,
Juha