* [Qemu-devel] [PATCH] timer/aspeed: fix timer enablement when a reload is not set
@ 2017-06-06 8:55 Cédric Le Goater
2017-06-09 2:26 ` Andrew Jeffery
2017-06-13 10:35 ` Peter Maydell
0 siblings, 2 replies; 6+ messages in thread
From: Cédric Le Goater @ 2017-06-06 8:55 UTC (permalink / raw)
To: Peter Maydell; +Cc: Andrew Jeffery, qemu-arm, qemu-devel, Cédric Le Goater
When a timer is enabled before a reload value is set, the controller
waits for a reload value to be set before starting decrementing. This
fix tries to cover that case by changing the timer expiry only when
a reload value is valid.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
hw/timer/aspeed_timer.c | 37 +++++++++++++++++++++++++++++--------
1 file changed, 29 insertions(+), 8 deletions(-)
diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 9b70ee09b07f..50acbf530a3a 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -130,15 +130,26 @@ static uint64_t calculate_next(struct AspeedTimer *t)
next = seq[1];
} else if (now < seq[2]) {
next = seq[2];
- } else {
+ } else if (t->reload) {
reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, rate);
t->start = now - ((now - t->start) % reload_ns);
+ } else {
+ /* no reload value, return 0 */
+ break;
}
}
return next;
}
+static void aspeed_timer_mod(AspeedTimer *t)
+{
+ uint64_t next = calculate_next(t);
+ if (next) {
+ timer_mod(&t->timer, next);
+ }
+}
+
static void aspeed_timer_expire(void *opaque)
{
AspeedTimer *t = opaque;
@@ -164,7 +175,7 @@ static void aspeed_timer_expire(void *opaque)
qemu_set_irq(t->irq, t->level);
}
- timer_mod(&t->timer, calculate_next(t));
+ aspeed_timer_mod(t);
}
static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
@@ -227,10 +238,23 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
uint32_t value)
{
AspeedTimer *t;
+ uint32_t old_reload;
trace_aspeed_timer_set_value(timer, reg, value);
t = &s->timers[timer];
switch (reg) {
+ case TIMER_REG_RELOAD:
+ old_reload = t->reload;
+ t->reload = value;
+
+ /* If the reload value was not previously set, or zero, and
+ * the current value is valid, try to start the timer if it is
+ * enabled.
+ */
+ if (old_reload || !t->reload) {
+ break;
+ }
+
case TIMER_REG_STATUS:
if (timer_enabled(t)) {
uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
@@ -238,17 +262,14 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
uint32_t rate = calculate_rate(t);
t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
- timer_mod(&t->timer, calculate_next(t));
+ aspeed_timer_mod(t);
}
break;
- case TIMER_REG_RELOAD:
- t->reload = value;
- break;
case TIMER_REG_MATCH_FIRST:
case TIMER_REG_MATCH_SECOND:
t->match[reg - 2] = value;
if (timer_enabled(t)) {
- timer_mod(&t->timer, calculate_next(t));
+ aspeed_timer_mod(t);
}
break;
default:
@@ -268,7 +289,7 @@ static void aspeed_timer_ctrl_enable(AspeedTimer *t, bool enable)
trace_aspeed_timer_ctrl_enable(t->id, enable);
if (enable) {
t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
- timer_mod(&t->timer, calculate_next(t));
+ aspeed_timer_mod(t);
} else {
timer_del(&t->timer);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] timer/aspeed: fix timer enablement when a reload is not set
2017-06-06 8:55 [Qemu-devel] [PATCH] timer/aspeed: fix timer enablement when a reload is not set Cédric Le Goater
@ 2017-06-09 2:26 ` Andrew Jeffery
2017-06-09 5:40 ` Cédric Le Goater
2017-06-13 10:35 ` Peter Maydell
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Jeffery @ 2017-06-09 2:26 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell; +Cc: qemu-arm, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4534 bytes --]
On Tue, 2017-06-06 at 10:55 +0200, Cédric Le Goater wrote:
> When a timer is enabled before a reload value is set, the controller
> waits for a reload value to be set before starting decrementing. This
> fix tries to cover that case by changing the timer expiry only when
> a reload value is valid.
>
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/timer/aspeed_timer.c | 37 +++++++++++++++++++++++++++++--------
> 1 file changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index 9b70ee09b07f..50acbf530a3a 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -130,15 +130,26 @@ static uint64_t calculate_next(struct AspeedTimer *t)
> next = seq[1];
> } else if (now < seq[2]) {
> next = seq[2];
> - } else {
> + } else if (t->reload) {
> reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, rate);
> t->start = now - ((now - t->start) % reload_ns);
> + } else {
> + /* no reload value, return 0 */
> + break;
> }
> }
>
> return next;
> }
>
> +static void aspeed_timer_mod(AspeedTimer *t)
> +{
> + uint64_t next = calculate_next(t);
> + if (next) {
> + timer_mod(&t->timer, next);
> + }
> +}
> +
> static void aspeed_timer_expire(void *opaque)
> {
> AspeedTimer *t = opaque;
> @@ -164,7 +175,7 @@ static void aspeed_timer_expire(void *opaque)
> qemu_set_irq(t->irq, t->level);
> }
>
> - timer_mod(&t->timer, calculate_next(t));
> + aspeed_timer_mod(t);
> }
>
> static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
> @@ -227,10 +238,23 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
> uint32_t value)
> {
> AspeedTimer *t;
> + uint32_t old_reload;
>
> trace_aspeed_timer_set_value(timer, reg, value);
> t = &s->timers[timer];
> switch (reg) {
> + case TIMER_REG_RELOAD:
> + old_reload = t->reload;
> + t->reload = value;
> +
> + /* If the reload value was not previously set, or zero, and
> + * the current value is valid, try to start the timer if it is
> + * enabled.
> + */
> + if (old_reload || !t->reload) {
> + break;
> + }
Maybe I need more caffeine, but I initially struggled to reconcile the
condition with the comment, as the condition checks the inverse in
order to break while the comment discusses the non-breaking case.
However, after trying for several minutes, I'm not sure there's an easy
way to improve it.
> +
> case TIMER_REG_STATUS:
> if (timer_enabled(t)) {
> uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> @@ -238,17 +262,14 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
> uint32_t rate = calculate_rate(t);
>
> t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
> - timer_mod(&t->timer, calculate_next(t));
> + aspeed_timer_mod(t);
> }
> break;
> - case TIMER_REG_RELOAD:
> - t->reload = value;
> - break;
> case TIMER_REG_MATCH_FIRST:
> case TIMER_REG_MATCH_SECOND:
> t->match[reg - 2] = value;
> if (timer_enabled(t)) {
> - timer_mod(&t->timer, calculate_next(t));
> + aspeed_timer_mod(t);
> }
> break;
> default:
> @@ -268,7 +289,7 @@ static void aspeed_timer_ctrl_enable(AspeedTimer *t, bool enable)
> trace_aspeed_timer_ctrl_enable(t->id, enable);
> if (enable) {
> t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> - timer_mod(&t->timer, calculate_next(t));
> + aspeed_timer_mod(t);
> } else {
> timer_del(&t->timer);
> }
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] timer/aspeed: fix timer enablement when a reload is not set
2017-06-09 2:26 ` Andrew Jeffery
@ 2017-06-09 5:40 ` Cédric Le Goater
2017-06-13 4:37 ` Andrew Jeffery
0 siblings, 1 reply; 6+ messages in thread
From: Cédric Le Goater @ 2017-06-09 5:40 UTC (permalink / raw)
To: Andrew Jeffery, Peter Maydell; +Cc: qemu-arm, qemu-devel
On 06/09/2017 04:26 AM, Andrew Jeffery wrote:
> On Tue, 2017-06-06 at 10:55 +0200, Cédric Le Goater wrote:
>> When a timer is enabled before a reload value is set, the controller
>> waits for a reload value to be set before starting decrementing. This
>> fix tries to cover that case by changing the timer expiry only when
>> a reload value is valid.
>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> hw/timer/aspeed_timer.c | 37 +++++++++++++++++++++++++++++--------
>> 1 file changed, 29 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
>> index 9b70ee09b07f..50acbf530a3a 100644
>> --- a/hw/timer/aspeed_timer.c
>> +++ b/hw/timer/aspeed_timer.c
>> @@ -130,15 +130,26 @@ static uint64_t calculate_next(struct AspeedTimer *t)
>> next = seq[1];
>> } else if (now < seq[2]) {
>> next = seq[2];
>> - } else {
>> + } else if (t->reload) {
>> reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, rate);
>> t->start = now - ((now - t->start) % reload_ns);
>> + } else {
>> + /* no reload value, return 0 */
>> + break;
>> }
>> }
>>
>> return next;
>> }
>>
>> +static void aspeed_timer_mod(AspeedTimer *t)
>> +{
>> + uint64_t next = calculate_next(t);
>> + if (next) {
>> + timer_mod(&t->timer, next);
>> + }
>> +}
>> +
>> static void aspeed_timer_expire(void *opaque)
>> {
>> AspeedTimer *t = opaque;
>> @@ -164,7 +175,7 @@ static void aspeed_timer_expire(void *opaque)
>> qemu_set_irq(t->irq, t->level);
>> }
>>
>> - timer_mod(&t->timer, calculate_next(t));
>> + aspeed_timer_mod(t);
>> }
>>
>> static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
>> @@ -227,10 +238,23 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
>> uint32_t value)
>> {
>> AspeedTimer *t;
>> + uint32_t old_reload;
>>
>> trace_aspeed_timer_set_value(timer, reg, value);
>> t = &s->timers[timer];
>> switch (reg) {
>> + case TIMER_REG_RELOAD:
>> + old_reload = t->reload;
>> + t->reload = value;
>> +
>> + /* If the reload value was not previously set, or zero, and
>> + * the current value is valid, try to start the timer if it is
>> + * enabled.
>> + */
>> + if (old_reload || !t->reload) {
>> + break;
>> + }
>
> Maybe I need more caffeine, but I initially struggled to reconcile the
> condition with the comment, as the condition checks the inverse in
> order to break while the comment discusses the non-breaking case.
I agree. The reload "value" is used in a hidden way to the activate the
timer.
> However, after trying for several minutes, I'm not sure there's an easy
> way to improve it.
I tried a few things. May be, we could move the following code in
its own routine and call it twice ?
>> +
>> case TIMER_REG_STATUS:
>> if (timer_enabled(t)) {
>> uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> @@ -238,17 +262,14 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
>> uint32_t rate = calculate_rate(t);
>>
>> t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
>> - timer_mod(&t->timer, calculate_next(t));
>> + aspeed_timer_mod(t);
>> }
>> break;
>> - case TIMER_REG_RELOAD:
>> - t->reload = value;
>> - break;
>> case TIMER_REG_MATCH_FIRST:
>> case TIMER_REG_MATCH_SECOND:
>> t->match[reg - 2] = value;
>> if (timer_enabled(t)) {
>> - timer_mod(&t->timer, calculate_next(t));
>> + aspeed_timer_mod(t);
>> }
>> break;
>> default:
>> @@ -268,7 +289,7 @@ static void aspeed_timer_ctrl_enable(AspeedTimer *t, bool enable)
>> trace_aspeed_timer_ctrl_enable(t->id, enable);
>> if (enable) {
>> t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> - timer_mod(&t->timer, calculate_next(t));
>> + aspeed_timer_mod(t);
>> } else {
>> timer_del(&t->timer);
>> }
>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
Thanks,
C.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] timer/aspeed: fix timer enablement when a reload is not set
2017-06-09 5:40 ` Cédric Le Goater
@ 2017-06-13 4:37 ` Andrew Jeffery
2017-06-13 5:28 ` Cédric Le Goater
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Jeffery @ 2017-06-13 4:37 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell; +Cc: qemu-arm, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5482 bytes --]
On Fri, 2017-06-09 at 07:40 +0200, Cédric Le Goater wrote:
> On 06/09/2017 04:26 AM, Andrew Jeffery wrote:
> > On Tue, 2017-06-06 at 10:55 +0200, Cédric Le Goater wrote:
> > > When a timer is enabled before a reload value is set, the controller
> > > waits for a reload value to be set before starting decrementing. This
> > > fix tries to cover that case by changing the timer expiry only when
> > > a reload value is valid.
> > >
> > > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > >
> > > ---
> > > hw/timer/aspeed_timer.c | 37 +++++++++++++++++++++++++++++--------
> > > 1 file changed, 29 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> > > index 9b70ee09b07f..50acbf530a3a 100644
> > > --- a/hw/timer/aspeed_timer.c
> > > +++ b/hw/timer/aspeed_timer.c
> > > @@ -130,15 +130,26 @@ static uint64_t calculate_next(struct AspeedTimer *t)
> > > next = seq[1];
> > > } else if (now < seq[2]) {
> > > next = seq[2];
> > > - } else {
> > > + } else if (t->reload) {
> > > reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, rate);
> > > t->start = now - ((now - t->start) % reload_ns);
> > > + } else {
> > > + /* no reload value, return 0 */
> > > + break;
> > > }
> > > }
> > >
> > > return next;
> > > }
> > >
> > > +static void aspeed_timer_mod(AspeedTimer *t)
> > > +{
> > > + uint64_t next = calculate_next(t);
> > > + if (next) {
> > > + timer_mod(&t->timer, next);
> > > + }
> > > +}
> > > +
> > > static void aspeed_timer_expire(void *opaque)
> > > {
> > > AspeedTimer *t = opaque;
> > > @@ -164,7 +175,7 @@ static void aspeed_timer_expire(void *opaque)
> > > qemu_set_irq(t->irq, t->level);
> > > }
> > >
> > > - timer_mod(&t->timer, calculate_next(t));
> > > + aspeed_timer_mod(t);
> > > }
> > >
> > > static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
> > > @@ -227,10 +238,23 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
> > > uint32_t value)
> > > {
> > > AspeedTimer *t;
> > > + uint32_t old_reload;
> > >
> > > trace_aspeed_timer_set_value(timer, reg, value);
> > > t = &s->timers[timer];
> > > switch (reg) {
> > > + case TIMER_REG_RELOAD:
> > > + old_reload = t->reload;
> > > + t->reload = value;
> > > +
> > > + /* If the reload value was not previously set, or zero, and
> > > + * the current value is valid, try to start the timer if it is
> > > + * enabled.
> > > + */
> > > + if (old_reload || !t->reload) {
> > > + break;
> > > + }
> >
> > Maybe I need more caffeine, but I initially struggled to reconcile the
> > condition with the comment, as the condition checks the inverse in
> > order to break while the comment discusses the non-breaking case.
>
> I agree. The reload "value" is used in a hidden way to the activate the
> timer.
>
> > However, after trying for several minutes, I'm not sure there's an easy
> > way to improve it.
>
> I tried a few things. May be, we could move the following code in
> its own routine and call it twice ?
I don't think it's necessary. The comment serves as enough warning - it
should at least make people think before modifying the code.
Cheers,
Andrew
>
> > > +
> > > case TIMER_REG_STATUS:
> > > if (timer_enabled(t)) {
> > > uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > > @@ -238,17 +262,14 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
> > > uint32_t rate = calculate_rate(t);
> > >
> > > t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
> > > - timer_mod(&t->timer, calculate_next(t));
> > > + aspeed_timer_mod(t);
> > > }
> > > break;
> > > - case TIMER_REG_RELOAD:
> > > - t->reload = value;
> > > - break;
> > > case TIMER_REG_MATCH_FIRST:
> > > case TIMER_REG_MATCH_SECOND:
> > > t->match[reg - 2] = value;
> > > if (timer_enabled(t)) {
> > > - timer_mod(&t->timer, calculate_next(t));
> > > + aspeed_timer_mod(t);
> > > }
> > > break;
> > > default:
> > > @@ -268,7 +289,7 @@ static void aspeed_timer_ctrl_enable(AspeedTimer *t, bool enable)
> > > trace_aspeed_timer_ctrl_enable(t->id, enable);
> > > if (enable) {
> > > t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > > - timer_mod(&t->timer, calculate_next(t));
> > > + aspeed_timer_mod(t);
> > > } else {
> > > timer_del(&t->timer);
> > > }
> >
> > Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>
> Thanks,
>
> C.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] timer/aspeed: fix timer enablement when a reload is not set
2017-06-13 4:37 ` Andrew Jeffery
@ 2017-06-13 5:28 ` Cédric Le Goater
0 siblings, 0 replies; 6+ messages in thread
From: Cédric Le Goater @ 2017-06-13 5:28 UTC (permalink / raw)
To: Andrew Jeffery, Peter Maydell; +Cc: qemu-arm, qemu-devel
On 06/13/2017 06:37 AM, Andrew Jeffery wrote:
> On Fri, 2017-06-09 at 07:40 +0200, Cédric Le Goater wrote:
>> On 06/09/2017 04:26 AM, Andrew Jeffery wrote:
>>> On Tue, 2017-06-06 at 10:55 +0200, Cédric Le Goater wrote:
>>>> When a timer is enabled before a reload value is set, the controller
>>>> waits for a reload value to be set before starting decrementing. This
>>>> fix tries to cover that case by changing the timer expiry only when
>>>> a reload value is valid.
>>>>
>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>
>>>> ---
>>>> hw/timer/aspeed_timer.c | 37 +++++++++++++++++++++++++++++--------
>>>> 1 file changed, 29 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
>>>> index 9b70ee09b07f..50acbf530a3a 100644
>>>> --- a/hw/timer/aspeed_timer.c
>>>> +++ b/hw/timer/aspeed_timer.c
>>>> @@ -130,15 +130,26 @@ static uint64_t calculate_next(struct AspeedTimer *t)
>>>> next = seq[1];
>>>> } else if (now < seq[2]) {
>>>> next = seq[2];
>>>> - } else {
>>>> + } else if (t->reload) {
>>>> reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, rate);
>>>> t->start = now - ((now - t->start) % reload_ns);
>>>> + } else {
>>>> + /* no reload value, return 0 */
>>>> + break;
>>>> }
>>>> }
>>>>
>>>> return next;
>>>> }
>>>>
>>>> +static void aspeed_timer_mod(AspeedTimer *t)
>>>> +{
>>>> + uint64_t next = calculate_next(t);
>>>> + if (next) {
>>>> + timer_mod(&t->timer, next);
>>>> + }
>>>> +}
>>>> +
>>>> static void aspeed_timer_expire(void *opaque)
>>>> {
>>>> AspeedTimer *t = opaque;
>>>> @@ -164,7 +175,7 @@ static void aspeed_timer_expire(void *opaque)
>>>> qemu_set_irq(t->irq, t->level);
>>>> }
>>>>
>>>> - timer_mod(&t->timer, calculate_next(t));
>>>> + aspeed_timer_mod(t);
>>>> }
>>>>
>>>> static uint64_t aspeed_timer_get_value(AspeedTimer *t, int reg)
>>>> @@ -227,10 +238,23 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
>>>> uint32_t value)
>>>> {
>>>> AspeedTimer *t;
>>>> + uint32_t old_reload;
>>>>
>>>> trace_aspeed_timer_set_value(timer, reg, value);
>>>> t = &s->timers[timer];
>>>> switch (reg) {
>>>> + case TIMER_REG_RELOAD:
>>>> + old_reload = t->reload;
>>>> + t->reload = value;
>>>> +
>>>> + /* If the reload value was not previously set, or zero, and
>>>> + * the current value is valid, try to start the timer if it is
>>>> + * enabled.
>>>> + */
>>>> + if (old_reload || !t->reload) {
>>>> + break;
>>>> + }
>>>
>>> Maybe I need more caffeine, but I initially struggled to reconcile the
>>> condition with the comment, as the condition checks the inverse in
>>> order to break while the comment discusses the non-breaking case.
>>
>> I agree. The reload "value" is used in a hidden way to the activate the
>> timer.
>>
>>> However, after trying for several minutes, I'm not sure there's an easy
>>> way to improve it.
>>
>> I tried a few things. May be, we could move the following code in
>> its own routine and call it twice ?
>
> I don't think it's necessary. The comment serves as enough warning - it
> should at least make people think before modifying the code.
OK. Let it be.
Peter,
The Moxa Art timer driver was recently merged into the Faraday
FTTMR010 driver and the initial setup is slightly different,
it enables the timer before setting the reload value, which
breaks the current QEMU model.
Thanks,
C.
> Cheers,
>
> Andrew
>
>>
>>>> +
>>>> case TIMER_REG_STATUS:
>>>> if (timer_enabled(t)) {
>>>> uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>>> @@ -238,17 +262,14 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
>>>> uint32_t rate = calculate_rate(t);
>>>>
>>>> t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
>>>> - timer_mod(&t->timer, calculate_next(t));
>>>> + aspeed_timer_mod(t);
>>>> }
>>>> break;
>>>> - case TIMER_REG_RELOAD:
>>>> - t->reload = value;
>>>> - break;
>>>> case TIMER_REG_MATCH_FIRST:
>>>> case TIMER_REG_MATCH_SECOND:
>>>> t->match[reg - 2] = value;
>>>> if (timer_enabled(t)) {
>>>> - timer_mod(&t->timer, calculate_next(t));
>>>> + aspeed_timer_mod(t);
>>>> }
>>>> break;
>>>> default:
>>>> @@ -268,7 +289,7 @@ static void aspeed_timer_ctrl_enable(AspeedTimer *t, bool enable)
>>>> trace_aspeed_timer_ctrl_enable(t->id, enable);
>>>> if (enable) {
>>>> t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>>> - timer_mod(&t->timer, calculate_next(t));
>>>> + aspeed_timer_mod(t);
>>>> } else {
>>>> timer_del(&t->timer);
>>>> }
>>>
>>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>>
>> Thanks,
>>
>> C.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] timer/aspeed: fix timer enablement when a reload is not set
2017-06-06 8:55 [Qemu-devel] [PATCH] timer/aspeed: fix timer enablement when a reload is not set Cédric Le Goater
2017-06-09 2:26 ` Andrew Jeffery
@ 2017-06-13 10:35 ` Peter Maydell
1 sibling, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2017-06-13 10:35 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: Andrew Jeffery, qemu-arm, QEMU Developers
On 6 June 2017 at 09:55, Cédric Le Goater <clg@kaod.org> wrote:
> When a timer is enabled before a reload value is set, the controller
> waits for a reload value to be set before starting decrementing. This
> fix tries to cover that case by changing the timer expiry only when
> a reload value is valid.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/timer/aspeed_timer.c | 37 +++++++++++++++++++++++++++++--------
> 1 file changed, 29 insertions(+), 8 deletions(-)
Applied to target-arm.next, thanks.
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-13 10:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-06 8:55 [Qemu-devel] [PATCH] timer/aspeed: fix timer enablement when a reload is not set Cédric Le Goater
2017-06-09 2:26 ` Andrew Jeffery
2017-06-09 5:40 ` Cédric Le Goater
2017-06-13 4:37 ` Andrew Jeffery
2017-06-13 5:28 ` Cédric Le Goater
2017-06-13 10:35 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.