Re: [PATCH 22/25] hw/arm/armsse: Use Clock to set system_clock_scale

[Luc Michel]

On 19:06 Thu 21 Jan     , Peter Maydell wrote:
> Use the MAINCLK Clock input to set the system_clock_scale variable
> rather than using the mainclk_frq property.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> At some point we should make the SysTick take a Clock itself so
> that we can get rid of the system_clock_scale global entirely.
> (In fact we want two Clocks: one that is the CPU clock and one
> for the 'external reference clock' whose period is currently
> hardcoded at 1000ns in systick_scale()...)
> ---
>  hw/arm/armsse.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
> index 4349ce9bfdb..1da0c1be4c7 100644
> --- a/hw/arm/armsse.c
> +++ b/hw/arm/armsse.c
> @@ -232,6 +232,16 @@ static void armsse_forward_sec_resp_cfg(ARMSSE *s)
>      qdev_connect_gpio_out(dev_splitter, 2, s->sec_resp_cfg_in);
>  }
>  
> +static void armsse_mainclk_update(void *opaque)
> +{
> +    ARMSSE *s = ARM_SSE(opaque);
> +    /*
> +     * Set system_clock_scale from our Clock input; this is what
> +     * controls the tick rate of the CPU SysTick timer.
> +     */
> +    system_clock_scale = clock_ticks_to_ns(s->mainclk, 1);
> +}
> +
I think you forgot to connect this callback to the clock itself (the
`qdev_init_clock_in` call in `armsse_init`).

Moreover on a clock change event, shouldn't the SysTick timer be
recomputed? I guess this will be better fixed in the SysTick itself once
it takes the two Clock inputs as you said.

-- 
Luc

>  static void armsse_init(Object *obj)
>  {
>      ARMSSE *s = ARM_SSE(obj);
> @@ -451,9 +461,11 @@ static void armsse_realize(DeviceState *dev, Error 
> **errp)
>          return;
>      }
>  
> -    if (!s->mainclk_frq) {
> -        error_setg(errp, "MAINCLK_FRQ property was not set");
> -        return;
> +    if (!clock_has_source(s->mainclk)) {
> +        error_setg(errp, "MAINCLK clock was not connected");
> +    }
> +    if (!clock_has_source(s->s32kclk)) {
> +        error_setg(errp, "S32KCLK clock was not connected");
>      }
>  
>      assert(info->num_cpus <= SSE_MAX_CPUS);
> @@ -1115,7 +1127,8 @@ static void armsse_realize(DeviceState *dev, Error 
> **errp)
>       */
>      sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->container);
>  
> -    system_clock_scale = NANOSECONDS_PER_SECOND / s->mainclk_frq;
> +    /* Set initial system_clock_scale from MAINCLK */
> +    armsse_mainclk_update(s);
>  }
>  
>  static void armsse_idau_check(IDAUInterface *ii, uint32_t address,
> -- 
> 2.20.1
> 

--