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

[Thomas Huth]

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;
                              ^

I think you should not re-typedef here. Either include the right header,
or use include/qemu/typedefs.h.

 Thomas


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