Re: [PATCH] hw/core/clock: always iterate through childs in clock_propagate_period

[Raphael Poggi]

Hi Philippe,

Le jeu. 18 avr. 2024 à 20:43, Philippe Mathieu-Daudé
<philmd@linaro.org> a écrit :
>
> Hi Raphael,
>
> On 18/4/24 21:16, Raphael Poggi wrote:
> > When dealing with few clocks depending with each others, sometimes
> > we might only want to update the multiplier/diviser on a specific clock
> > (cf clockB in drawing below) and call "clock_propagate(clockA)" to
> > update the childs period according to the potential new multiplier/diviser 
> > values.
> >
> > +--------+     +--------+      +--------+
> > | clockA | --> | clockB |  --> | clockC |
> > +--------+     +--------+      +--------+
> >
> > The actual code would not allow that because, since we cannot call
> > "clock_propagate" directly on a child, it would exit on the
> > first child has the period has not changed for clockB, only clockC is
>
> Typo "as the period has not changed"?

That's a typo indeed, thanks!

>
> Why can't you call clock_propagate() on a child?

There is an assert "assert(clk->source == NULL);" in clock_propagate().
If I am not wrong, clk->source is set when the clock has a parent.

>
> > impacted in our example.
> >
> > Signed-off-by: Raphael Poggi <raphael.poggi@lynxleap.co.uk>
> > ---
> >   hw/core/clock.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/core/clock.c b/hw/core/clock.c
> > index a19c7db7df..85421f8b55 100644
> > --- a/hw/core/clock.c
> > +++ b/hw/core/clock.c
> > @@ -101,8 +101,9 @@ static void clock_propagate_period(Clock *clk, bool 
> > call_callbacks)
> >               if (call_callbacks) {
> >                   clock_call_callback(child, ClockUpdate);
> >               }
> > -            clock_propagate_period(child, call_callbacks);
> >           }
> > +
> > +        clock_propagate_period(child, call_callbacks);
> >       }
> >   }
> >
>