| Message ID | 1445608598-24485-13-git-send-email-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
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. > > 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? > 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? > qemu_get_timedate(&tm, 0); > s->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
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 <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. >> 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.
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?
On 11/11/15 06:52, David Gibson wrote: > 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. Okay I've added a reference to MacOS 9 in the updated commit message. >> >>>> >>>> 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. Done. >>>> 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? Yes, my understanding from studying the code is that both timers run off a fixed frequency clock with the difference being that T1 allows both the counter and latch to be updated (with an optional continuous mode), while T2 is free running. Both timers can be configured to interrupt just after the zero-crossing point if required. ATB, Mark.
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); 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; qemu_get_timedate(&tm, 0); s->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
Fix the counter loading logic and enable the T2 interrupt when the timer expires. 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(-)