qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Luc Michel
Subject: Re: [PATCH-for-9.0 2/2] hw/misc/stm32l4x5_rcc: Propagate period when enabling a clock
Date: Mon, 25 Mar 2024 09:56:39 +0100

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,

-- 
Luc

> 
> (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

-- 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]