[PATCH] hw/core/clock: remove assert in clock_propagate
This commit allows childs clock to propagate their new frequency,
for example, after setting a new multiplier/diviser.
Signed-off-by: Raphael Poggi
---
hw/core/clock.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/core/clock.c b/hw/core/clock.c
index 85421f8b55..174c8be095 100644
--- a/hw/core/clock.c
+++ b/hw/core/clock.c
@@ -109,7 +109,6 @@ static void clock_propagate_period(Clock *clk, bool
call_callbacks)
void clock_propagate(Clock *clk)
{
-assert(clk->source == NULL);
trace_clock_propagate(CLOCK_PATH(clk));
clock_propagate_period(clk, true);
}
--
2.44.0
Re: [PATCH] hw/core/clock: always iterate through childs in clock_propagate_period
Hi Peter, Le ven. 19 avr. 2024 à 16:08, Peter Maydell a écrit : > > On Thu, 18 Apr 2024 at 21:39, Raphael Poggi > wrote: > > > > Hi Philippe, > > > > Le jeu. 18 avr. 2024 à 20:43, Philippe Mathieu-Daudé > > 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. > > I think that assertion is probably there because we didn't > originally have the idea of a clock having a multiplier/divider > setting. So the idea was that calling clock_propagate() on a > clock with a parent would always be wrong, because the only > reason for its period to change would be if the parent had > changed, and if the parent changes then clock_propagate() > should be called on the parent. > > We added mul/div later, and we (I) didn't think through all > the consequences. If you change the mul/div settings on > clockB in this example then you need to call clock_propagate() > on it, so we should remove that assert(). Then when you change > the mul/div on clockB you can directly clock_propagate(clockB), > and I don't think you need this patch at that point. Alright, that makes sense, is that OK if I send a patch removing the assert ? Thanks, > > thanks > -- PMM
Re: [PATCH] hw/core/clock: always iterate through childs in clock_propagate_period
Hi Philippe,
Le jeu. 18 avr. 2024 à 20:43, Philippe Mathieu-Daudé
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
> > ---
> > 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);
> > }
> > }
> >
>
[PATCH] hw/core/clock: always iterate through childs in clock_propagate_period
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
impacted in our example.
Signed-off-by: Raphael Poggi
---
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);
}
}
--
2.44.0
