diff mbox series

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

Message ID 20240322155810.5733-3-philmd@linaro.org
State New
Headers show
Series hw/clock: Propagate clock changes when STM32L4X5 MUX is updated | expand

Commit Message

Philippe Mathieu-Daudé March 22, 2024, 3:58 p.m. UTC 
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(-)

Comments

Peter Maydell March 22, 2024, 4:39 p.m. UTC | #1 
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
Luc Michel March 25, 2024, 8:56 a.m. UTC | #2 
On 16:39 Fri 22 Mar     , Peter Maydell wrote:
> 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...)
clock_set() does not call clock_propagate(). clock_update() is a
clock_set() followed by a clock_propagate() if the period changed.

I think this is where the problem comes from here. clock_update() call
won't call clock_propagate() if the clock period does not change.

I think you'll want something like:
    bool changed;

    changed = clock_set_mul_div(mux->out, freq_multiplier, mux->multiplexer);
    changed ||= clock_set(clock_get(current_source));

    if (changed) {
        clock_propagate(mux->out);
    }
 
Thanks,
diff mbox series

Patch

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));
 
     src_freq = clock_get_hz(current_source);