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