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

[Philippe Mathieu-Daudé]

On 1/21/21 10:59 PM, Philippe Mathieu-Daudé wrote:
> 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.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>