Re: [PATCH 17/25] hw/arm/stellaris: Create Clock input for watchdog

[Philippe Mathieu-Daudé]

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.maydell@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.