Re: [RFC PATCH-for-6.1 9/9] hw/core/machine: Reset machine clocks using qemu_register_reset()

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

On Fri, 9 Apr 2021 at 07:24, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> While the documentation mentions:
>
>   Note that if you are creating a clock with a fixed period which
>   will never change (for example the main clock source of a board),
>   then you'll have nothing else to do. This value will be propagated
>   to other clocks when connecting the clocks together and devices
>   will fetch the right value during the first reset.
>
> the clocks created in machine_init() aren't propagating their value
> because they are never reset (not part of the reset tree, such
> TYPE_DEVICE).
>
> Register a generic reset handler to have them properly reset.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/core/machine.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index e8bdcd10854..2817fe6a567 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1234,6 +1234,13 @@ void machine_run_board_init(MachineState *machine)
>      phase_advance(PHASE_MACHINE_INITIALIZED);
>  }
>
> +static void constant_clock_reset(void *opaque)
> +{
> +    Clock *clk = opaque;
> +
> +    clock_propagate(clk);
> +}
> +
>  Clock *machine_create_constant_clock(MachineState *machine,
>                                       const char *name, unsigned freq_hz)
>  {
> @@ -1241,6 +1248,7 @@ Clock *machine_create_constant_clock(MachineState *machine,
>
>      clk = clock_new(OBJECT(machine), name);
>      clock_set_hz(clk, freq_hz);
> +    qemu_register_reset(constant_clock_reset, clk);

You mention this in the cover letter, but I agree that this
isn't really very nice. The machine's reset method ought to
reset the clocks (either explicitly or maybe some day implicitly).

thanks
-- PMM