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

[Cédric Le Goater]

On 08/31/2018 10:34 AM, Thomas Huth wrote:
> On 2018-06-22 09:57, Cédric Le Goater wrote:
>> The timer controller can be driven by either an external 1MHz clock or
>> by the APB clock. Today, the model makes the assumption that the APB
>> frequency is always set to 24MHz but this is incorrect.
>>
>> The AST2400 SoC on the palmetto machines uses a 48MHz input clock
>> source and the APB can be set to 48MHz. The consequence is a general
>> system slowdown. The QEMU machines using the AST2500 SoC do not seem
>> impacted today because the APB frequency is still set to 24MHz.
>>
>> We fix the timer frequency for all SoCs by linking the Timer model to
>> the SCU model. The APB frequency driving the timers is now the one
>> configured for the SoC.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> Reviewed-by: Joel Stanley <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;
> 
> This patch breaks compiling with clang 3.4.2 for me:
> 
> In file included from /home/thuth/devel/qemu/hw/timer/aspeed_timer.c:16:
> /home/thuth/devel/qemu/include/hw/misc/aspeed_scu.h:37:3: error:
> redefinition of typedef 'AspeedSCUState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> } AspeedSCUState;
>   ^
> /home/thuth/devel/qemu/include/hw/timer/aspeed_timer.h:27:31: note:
> previous definition is here
> typedef struct AspeedSCUState AspeedSCUState;

Ah. Bummer. I will just include the file then.
                              
> 
> I think you should not re-typedef here. Either include the right header,
> or use include/qemu/typedefs.h.

I didn't know this file existed. I am not sure to like this method.

>  Thomas
> 
> 
> PS: Did I already mention that I really dislike the typedeffing in QEMU?
> 

It's a bit painful I agree.

Thanks,

C.