Hi Peter,
On 1/21/21 8:06 PM, Peter Maydell wrote:
> Create and connect the Clock input for the watchdog device on the
> Stellaris boards. Because the Stellaris boards model the ability to
> change the clock rate by programming PLL registers, we have to create
> an output Clock on the ssys_state device and wire it up to the
> watchdog.
>
> Note that the old comment on ssys_calculate_system_clock() got the
> units wrong -- system_clock_scale is in nanoseconds, not
> milliseconds. Improve the commentary to clarify how we are
> calculating the period.
>
> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
> ---
> hw/arm/stellaris.c | 43 +++++++++++++++++++++++++++++++------------
> 1 file changed, 31 insertions(+), 12 deletions(-)
...
> /*
> - * Caculate the sys. clock period in ms.
> + * Calculate the system clock period. We only want to propagate
> + * this change to the rest of the system if we're not being called
> + * from migration post-load.
This part was not trivial to understand. I read the Clock API
doc again then found:
Care should be taken not to use ``clock_update[_ns|_hz]()`` or
``clock_propagate()`` during the whole migration procedure because it
will trigger side effects to other devices in an unknown state.
> */
> -static void ssys_calculate_system_clock(ssys_state *s)
> +static void ssys_calculate_system_clock(ssys_state *s, bool propagate_clock)
> {
> + /*
> + * SYSDIV field specifies divisor: 0 == /1, 1 == /2, etc. Input
> + * clock is 200MHz, which is a period of 5 ns. Dividing the clock
> + * frequency by X is the same as multiplying the period by X.
> + */
> if (ssys_use_rcc2(s)) {
> system_clock_scale = 5 * (((s->rcc2 >> 23) & 0x3f) + 1);
> } else {
> system_clock_scale = 5 * (((s->rcc >> 23) & 0xf) + 1);
> }
> + clock_set_ns(s->sysclk, system_clock_scale);
> + if (propagate_clock) {
> + clock_propagate(s->sysclk);
> + }
> }
...
> static void stellaris_sys_reset_exit(Object *obj)
> @@ -690,7 +704,7 @@ static int stellaris_sys_post_load(void *opaque, int
> version_id)
> {
> ssys_state *s = opaque;
>
> - ssys_calculate_system_clock(s);
> + ssys_calculate_system_clock(s, false);
So this makes sense.
I'll keep reviewing and come back to this patch later.
Regards,
Phil.
