| Message ID | 20210121190622.22000-23-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Convert CMSDK timer, watchdog, dualtimer to Clock framework | expand |
On 1/21/21 8:06 PM, 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(-) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
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 > --
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
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); +} + 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,
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(-) -- 2.20.1