Re: [Qemu-devel] [PATCH 12/13] cuda.c: fix T2 timer and enable its interrupt

[Mark Cave-Ayland]

On 04/11/15 03:40, David Gibson wrote:

> On Fri, Oct 23, 2015 at 02:56:37PM +0100, Mark Cave-Ayland wrote:
>> Fix the counter loading logic and enable the T2 interrupt when the timer
>> expires.
> 
> A mention of what uses T2, and therefore why this is useful would be
> good.

There is a good chance that nothing has used T2 before MacOS 9 since
before this patch, it is impossible for the T2 timer interrupt to fire.
It can be seen that MacOS 9 does write to the relevant registers during
boot, and if the T2 interrupt is disabled then boot will hang.

>>
>> Signed-off-by: Mark Cave-Ayland <address@hidden>
>> ---
>>  hw/misc/macio/cuda.c |   30 +++++++++++++++++++++---------
>>  1 file changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>> index 687cb54..d864b24 100644
>> --- a/hw/misc/macio/cuda.c
>> +++ b/hw/misc/macio/cuda.c
>> @@ -136,7 +136,7 @@ static void cuda_timer_update(CUDAState *s, CUDATimer 
>> *ti,
>>  
>>  static void cuda_update_irq(CUDAState *s)
>>  {
>> -    if (s->ifr & s->ier & (SR_INT | T1_INT)) {
>> +    if (s->ifr & s->ier & (SR_INT | T1_INT | T2_INT)) {
>>          qemu_irq_raise(s->irq);
>>      } else {
>>          qemu_irq_lower(s->irq);
>> @@ -175,7 +175,7 @@ static unsigned int get_counter(CUDATimer *ti)
>>  
>>  static void set_counter(CUDAState *s, CUDATimer *ti, unsigned int val)
>>  {
>> -    CUDA_DPRINTF("T%d.counter=%d\n", 1 + (ti->timer == NULL), val);
>> +    CUDA_DPRINTF("T%d.counter=%d\n", 1 + ti->index, val);
>>      ti->load_time = get_tb(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
>>                             s->frequency);
>>      ti->counter_value = val;
>> @@ -220,7 +220,7 @@ static void cuda_timer_update(CUDAState *s, CUDATimer 
>> *ti,
>>  {
>>      if (!ti->timer)
>>          return;
>> -    if ((s->acr & T1MODE) != T1MODE_CONT) {
>> +    if (ti->index == 0 && (s->acr & T1MODE) != T1MODE_CONT) {
>>          timer_del(ti->timer);
>>      } else {
>>          ti->next_irq_time = get_next_irq_time(ti, current_time);
>> @@ -238,6 +238,16 @@ static void cuda_timer1(void *opaque)
>>      cuda_update_irq(s);
>>  }
>>  
>> +static void cuda_timer2(void *opaque)
>> +{
>> +    CUDAState *s = opaque;
>> +    CUDATimer *ti = &s->timers[1];
>> +
>> +    cuda_timer_update(s, ti, ti->next_irq_time);
>> +    s->ifr |= T2_INT;
>> +    cuda_update_irq(s);
>> +}
>> +
>>  static uint32_t cuda_readb(void *opaque, hwaddr addr)
>>  {
>>      CUDAState *s = opaque;
>> @@ -276,6 +286,7 @@ static uint32_t cuda_readb(void *opaque, hwaddr addr)
>>      case CUDA_REG_T2CL:
>>          val = get_counter(&s->timers[1]) & 0xff;
>>          s->ifr &= ~T2_INT;
>> +        cuda_update_irq(s);
>>          break;
>>      case CUDA_REG_T2CH:
>>          val = get_counter(&s->timers[1]) >> 8;
>> @@ -352,11 +363,12 @@ static void cuda_writeb(void *opaque, hwaddr addr, 
>> uint32_t val)
>>          cuda_timer_update(s, &s->timers[0], 
>> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
>>          break;
>>      case CUDA_REG_T2CL:
>> -        s->timers[1].latch = val;
>> -        set_counter(s, &s->timers[1], val);
>> +        s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
>>          break;
>>      case CUDA_REG_T2CH:
>> -        set_counter(s, &s->timers[1], (val << 8) | s->timers[1].latch);
>> +        s->timers[1].latch = (s->timers[1].latch & 0xff) | (val << 8);
>> +        s->ifr &= ~T2_INT;
>> +        set_counter(s, &s->timers[1], s->timers[1].latch);
> 
> So the new code appears to be like that for T1CL / T1CH, which makes
> sense.  However, T1CL has a cuda_timer_update() call.  Do you also
> need that for T2CL?

This is a side-effect of combining the T1 and T2 code. Unlike T1, T2
appears to be free-running from its written value but generating an
interrupt just after zero-crossing. So in order to get the correct
interval, we write the value to the latch instead of the counter to get
the same effect with the shared timer code.

>>          break;
>>      case CUDA_REG_SR:
>>          s->sr = val;
>> @@ -719,8 +731,7 @@ static void cuda_reset(DeviceState *dev)
>>      s->timers[0].latch = 0xffff;
>>      set_counter(s, &s->timers[0], 0xffff);
>>  
>> -    s->timers[1].latch = 0;
>> -    set_counter(s, &s->timers[1], 0xffff);
>> +    s->timers[1].latch = 0xffff;
>>  }
>>  
>>  static void cuda_realizefn(DeviceState *dev, Error **errp)
>> @@ -730,7 +741,8 @@ static void cuda_realizefn(DeviceState *dev, Error 
>> **errp)
>>  
>>      s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_timer1, s);
>>      s->timers[0].frequency = s->frequency;
>> -    s->timers[1].frequency = s->frequency;
>> +    s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_timer2, s);
>> +    s->timers[1].frequency = (SCALE_US * 6000) / 4700;
> 
> Where does this T2 frequency come from?

My understanding of this is that with the shared timer code, the IRQ
timing is calculated based upon CUDA_TIMER_FREQ (4.7MHz / 6). Therefore
by setting the frequency to the inverse value ((SCALE_US * 6000) / 4700)
then this cancels out the effect of the timebase calculation algorithm
used in the counters. I believe this came from Alex so he would likely
be able to clarify this and give a much better explanation.

>>      qemu_get_timedate(&tm, 0);
>>      s->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
> 


ATB,

Mark.