Re: [PATCH-for-9.0 2/2] hw/misc/stm32l4x5_rcc: Propagate period when enabling a clock

[Peter Maydell]

On Fri, 22 Mar 2024 at 15:59, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> From: Arnaud Minier <arnaud.minier@telecom-paris.fr>
>
> The "clock_set_mul_div" function doesn't propagate the clock period
> to the children if it is changed (e.g. by enabling/disabling a clock
> multiplexer).
> This was overlooked during the implementation due to late changes.
>
> This commit propagates the change if the multiplier or divider changes.
>
> Fixes: ec7d83acbd ("hw/misc/stm32l4x5_rcc: Add an internal clock multiplexer 
> object")
> Signed-off-by: Arnaud Minier <arnaud.minier@telecom-paris.fr>
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> Message-ID: <20240317103918.44375-2-arnaud.minier@telecom-paris.fr>
> [PMD: Check clock_set_mul_div() return value]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/misc/stm32l4x5_rcc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/misc/stm32l4x5_rcc.c b/hw/misc/stm32l4x5_rcc.c
> index bc2d63528b..7ad628b296 100644
> --- a/hw/misc/stm32l4x5_rcc.c
> +++ b/hw/misc/stm32l4x5_rcc.c
> @@ -59,7 +59,10 @@ static void clock_mux_update(RccClockMuxState *mux, bool 
> bypass_source)
>          freq_multiplier = mux->divider;
>      }
>
> -    clock_set_mul_div(mux->out, freq_multiplier, mux->multiplier);
> +    if (clock_set_mul_div(mux->out, freq_multiplier, mux->multiplier)) {
> +        clock_propagate(mux->out);
> +    }
> +
>      clock_update(mux->out, clock_get(current_source));

clock_update() also calls clock_propagate(), so this doesn't
seem entirely right: shouldn't we figure out whether we need to
do a clock_propagate() and do it once? (Maybe what seems odd to me
is that clock_set() does clock_propagate() for you but
clock_set_mul_div() does not...)

(Also I think we should have the information we need now to be able
to do the "reduce log spam" in the comment -- if neither
clock_set_mul_div() nor clock_update() needed to do anything
then we didn't actually change the config.)

-- PMM