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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6626 bytes --]

On Wed, Nov 04, 2015 at 11:25:43PM +0000, Mark Cave-Ayland wrote:
> 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.

Sorry, I wasn't clear.  My point is that anyone looking back on this
commit in the future won't necessarily have the context of the series
its part of here.  So, the individual commit message should explain
that T2 is being enabled in order to support MacOS9.

*I* know this is about MacOS9 enablement, but that's not clear from the
commit & message itself.

> 
> >>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> ---
> >>  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.

Ok.  I think you need to put that paragraph into a comment - without
that subtle background information, it looks like an obvious bug.

> >>          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.

Hmm, yeah, I'm having trouble following that.  I won't hold the series
up over it, but a clearer rationale would help here.

I take it that T2 has a fixed frequency, whereas T1 is adjustable,
based on the code snippet above?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]