Re: [Qemu-devel] [PATCH 3/3] aspeed/timer: use the APB frequency from the SCU

[Cédric Le Goater]

On 06/22/2018 03:01 AM, Joel Stanley wrote:
> On 22 June 2018 at 08:09, Cédric Le Goater <address@hidden> wrote:
>> The timer controller can be driven by either an external 1MHz clock or
>> by the APB clock. Today, the model makes this assumption that the APB
>> frequency is 24MHz but this is incorrect on the AST2400 SoC. palmetto
>> machines use a 48MHz input clock source.
> 
> So this fixes it to use a apb frequency from the scu for all platform,
> not just the 2400?

Yes. This is true all, platforms are impacted. The AST2500 settings 
are still using the default 24MHz freq emulated by QEMU. So they seem
not impacted but they are. I will update the commit log.

> Can you put something in the commit message about the change in guest
> behaviour - the output of Linux's clk_summary or similar - so we know
> what bug this fixed?

Ah ! the command 'sleep 1' does now effectively last one second and 
not two. Difficult to show in an email :) 

> The code looks good though.
> 
> Reviewed-by: Joel Stanley <address@hidden>

Thanks,

C.
 
>> Use the SCU object to get the APB frequency and drive the timers with
>> the configured freq for the SoC.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  include/hw/timer/aspeed_timer.h |  4 ++++
>>  hw/arm/aspeed_soc.c             |  2 ++
>>  hw/timer/aspeed_timer.c         | 19 +++++++++++++++----
>>  3 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/hw/timer/aspeed_timer.h 
>> b/include/hw/timer/aspeed_timer.h
>> index bd6c1a7f9609..040a08873432 100644
>> --- a/include/hw/timer/aspeed_timer.h
>> +++ b/include/hw/timer/aspeed_timer.h
>> @@ -24,6 +24,8 @@
>>
>>  #include "qemu/timer.h"
>>
>> +typedef struct AspeedSCUState AspeedSCUState;
>> +
>>  #define ASPEED_TIMER(obj) \
>>      OBJECT_CHECK(AspeedTimerCtrlState, (obj), TYPE_ASPEED_TIMER);
>>  #define TYPE_ASPEED_TIMER "aspeed.timer"
>> @@ -55,6 +57,8 @@ typedef struct AspeedTimerCtrlState {
>>      uint32_t ctrl;
>>      uint32_t ctrl2;
>>      AspeedTimer timers[ASPEED_TIMER_NR_TIMERS];
>> +
>> +    AspeedSCUState *scu;
>>  } AspeedTimerCtrlState;
>>
>>  #endif /* ASPEED_TIMER_H */
>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>> index 7cc05ee27ea4..e68911af0f90 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -127,6 +127,8 @@ static void aspeed_soc_init(Object *obj)
>>
>>      object_initialize(&s->timerctrl, sizeof(s->timerctrl), 
>> TYPE_ASPEED_TIMER);
>>      object_property_add_child(obj, "timerctrl", OBJECT(&s->timerctrl), 
>> NULL);
>> +    object_property_add_const_link(OBJECT(&s->timerctrl), "scu",
>> +                                   OBJECT(&s->scu), &error_abort);
>>      qdev_set_parent_bus(DEVICE(&s->timerctrl), sysbus_get_default());
>>
>>      object_initialize(&s->i2c, sizeof(s->i2c), TYPE_ASPEED_I2C);
>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
>> index 1e31e22b6f1f..5e3f51b66b43 100644
>> --- a/hw/timer/aspeed_timer.c
>> +++ b/hw/timer/aspeed_timer.c
>> @@ -10,8 +10,10 @@
>>   */
>>
>>  #include "qemu/osdep.h"
>> +#include "qapi/error.h"
>>  #include "hw/sysbus.h"
>>  #include "hw/timer/aspeed_timer.h"
>> +#include "hw/misc/aspeed_scu.h"
>>  #include "qemu-common.h"
>>  #include "qemu/bitops.h"
>>  #include "qemu/timer.h"
>> @@ -26,7 +28,6 @@
>>  #define TIMER_CLOCK_USE_EXT true
>>  #define TIMER_CLOCK_EXT_HZ 1000000
>>  #define TIMER_CLOCK_USE_APB false
>> -#define TIMER_CLOCK_APB_HZ 24000000
>>
>>  #define TIMER_REG_STATUS 0
>>  #define TIMER_REG_RELOAD 1
>> @@ -80,11 +81,11 @@ static inline bool timer_external_clock(AspeedTimer *t)
>>      return timer_ctrl_status(t, op_external_clock);
>>  }
>>
>> -static uint32_t clock_rates[] = { TIMER_CLOCK_APB_HZ, TIMER_CLOCK_EXT_HZ };
>> -
>>  static inline uint32_t calculate_rate(struct AspeedTimer *t)
>>  {
>> -    return clock_rates[timer_external_clock(t)];
>> +    AspeedTimerCtrlState *s = timer_to_ctrl(t);
>> +
>> +    return timer_external_clock(t) ? TIMER_CLOCK_EXT_HZ : s->scu->apb_freq;
>>  }
>>
>>  static inline uint32_t calculate_ticks(struct AspeedTimer *t, uint64_t 
>> now_ns)
>> @@ -449,6 +450,16 @@ static void aspeed_timer_realize(DeviceState *dev, 
>> Error **errp)
>>      int i;
>>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>      AspeedTimerCtrlState *s = ASPEED_TIMER(dev);
>> +    Object *obj;
>> +    Error *err = NULL;
>> +
>> +    obj = object_property_get_link(OBJECT(dev), "scu", &err);
>> +    if (!obj) {
>> +        error_propagate(errp, err);
>> +        error_prepend(errp, "required link 'scu' not found: ");
>> +        return;
>> +    }
>> +    s->scu = ASPEED_SCU(obj);
>>
>>      for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
>>          aspeed_init_one_timer(s, i);
>> --
>> 2.13.6
>>