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

[Peter Maydell <peter.maydell@linaro.org>]

On Sat, 23 Jan 2021 at 20:30, Luc Michel <luc@lmichel.fr> wrote:
>
> 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`).

Whoops, yes :-) As it happens all ARMSSE users are fixed-frequency
so this doesn't make a guest-visible difference, but it was
definitely my intention that this was a clock update callback.

> Moreover on a clock change event, shouldn't the SysTick timer be
> recomputed?

Systick correctly handles the system_clock_scale global being changed
at runtime, so once this callback is actually wired up to its Clock
that should just work.

thanks
-- PMM