[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